[PATCH v3 0/4] cmd: bootefi: refactor the code for bootmgr

[PATCH v3 0/4] cmd: bootefi: refactor the code for bootmgr

[AKASHI Takahiro]

This patch set is motivated by the discussion[1] regarding
CONFIG_BOOTEFI_BOOTMGR option.

# It has been partially merged in -next branch. So this version (v3)
# contains only the remaining commits.

At the end, bootefi.c will be decomposed into two parts, one for
providing the command itself and one for implementing helper functions.
EFI_LOADER will now be available without CONFIG_CMDLINE or specifically
CONFIG_CMD_BOOTEFI if invoked via bootmeth/bootstd.

Then, EFI_LOADER library side will be further split into two options
for fine-grain control:
CONFIG_EFI_BINARY_EXEC: execute UEFI binaries which are to be explicitly
    loaded by U-Boot's load commands/functions or other methods
    (like a jtag debugger?)
    It supports bootmeth_efi as well as "bootefi <addr>|hello"(/"bootm"?).

CONFIG_EFI_BOOTMGR: provide EFI boot manger functionality
    It supports bootmeth_efi_mgr as well as "bootefi bootmgr".

As such, We will no longer need CONFIG_EFI_BINARY_EXEC if we want to only
make use of the UEFI boot manger for booting a next stage OS.

Prerequisite
============
This patch set is based on top of Tom's latest "next" branch.

Tests
=====
* run UT efi_selftest on sandbox locally
* run test_efi_bootmgr on sandbox locally

Unfortunately, I could not submit a pull request for CI test.

Changes
=======
v3 (Dec 18, 2023)
* rebased onto Tom's latest next branch
* remove already-merged commits

v2 (Nov 21, 2023)
* rebased onto Tom's next branch
* remove already merged commits
* revise commit messages
* add patch #5 which was split from ex-patch#5
RFC (Oct 26, 2023)

[1] https://lists.denx.de/pipermail/u-boot/2023-October/534598.html

AKASHI Takahiro (4):
  efi_loader: split unrelated code from efi_bootmgr.c
  efi_loader: rename BOOTEFI_BOOTMGR to EFI_BOOTMGR
  net: tftp: remove explicit efi configuration dependency
  fs: remove explicit efi configuration dependency

 boot/Kconfig                     |   4 +-
 boot/Makefile                    |   2 +-
 cmd/Kconfig                      |  10 +-
 cmd/efidebug.c                   |   4 +-
 fs/fs.c                          |   7 +-
 include/efi_loader.h             |  28 +-
 lib/efi_loader/Kconfig           |  11 +-
 lib/efi_loader/Makefile          |   2 +-
 lib/efi_loader/efi_bootmgr.c     | 493 ------------------------------
 lib/efi_loader/efi_device_path.c |   3 +-
 lib/efi_loader/efi_helper.c      | 499 ++++++++++++++++++++++++++++++-
 net/tftp.c                       |  10 +-
 test/boot/bootflow.c             |   2 +-
 13 files changed, 544 insertions(+), 531 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c

[AKASHI Takahiro]

Some code moved from cmd/bootefi.c is actually necessary only for "bootefi
<addr>" command (starting an image manually loaded by a user using U-Boot
load commands or other methods (like JTAG debugger).

The code will never been opted out as unused code by a compiler which
doesn't know how EFI boot manager is implemented. So introduce a new
configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out
explicitly.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 boot/Kconfig                     |   4 +-
 cmd/Kconfig                      |   6 +-
 include/efi_loader.h             |  28 +-
 lib/efi_loader/Kconfig           |   9 +
 lib/efi_loader/efi_bootmgr.c     | 493 ------------------------------
 lib/efi_loader/efi_device_path.c |   3 +-
 lib/efi_loader/efi_helper.c      | 499 ++++++++++++++++++++++++++++++-
 7 files changed, 529 insertions(+), 513 deletions(-)

diff --git a/boot/Kconfig b/boot/Kconfig
index 987ca7314117..8ab7e6f63d34 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -523,7 +523,7 @@ config BOOTMETH_EXTLINUX_PXE
 
 config BOOTMETH_EFILOADER
 	bool "Bootdev support for EFI boot"
-	depends on BOOTEFI_BOOTMGR
+	depends on EFI_BINARY_EXEC
 	default y
 	help
 	  Enables support for EFI boot using bootdevs. This makes the
@@ -558,7 +558,7 @@ config BOOTMETH_DISTRO
 	select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
 	select BOOTMETH_EXTLINUX  # E.g. Debian uses these
 	select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
-	select BOOTMETH_EFILOADER if BOOTEFI_BOOTMGR # E.g. Ubuntu uses this
+	select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
 
 config SPL_BOOTMETH_VBE
 	bool "Bootdev support for Verified Boot for Embedded (SPL)"
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 24bfbe505722..2c993496b70e 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -273,7 +273,7 @@ config CMD_BOOTMETH
 
 config BOOTM_EFI
 	bool "Support booting UEFI FIT images"
-	depends on BOOTEFI_BOOTMGR && CMD_BOOTM && FIT
+	depends on EFI_BINARY_EXEC && CMD_BOOTM && FIT
 	default y
 	help
 	  Support booting UEFI FIT images via the bootm command.
@@ -365,7 +365,7 @@ config CMD_BOOTEFI
 if CMD_BOOTEFI
 config CMD_BOOTEFI_BINARY
 	bool "Allow booting an EFI binary directly"
-	depends on BOOTEFI_BOOTMGR
+	depends on EFI_BINARY_EXEC
 	default y
 	help
 	  Select this option to enable direct execution of binary at 'bootefi'.
@@ -395,7 +395,7 @@ config CMD_BOOTEFI_HELLO_COMPILE
 
 config CMD_BOOTEFI_HELLO
 	bool "Allow booting a standard EFI hello world for testing"
-	depends on CMD_BOOTEFI_HELLO_COMPILE
+	depends on CMD_BOOTEFI_BINARY && CMD_BOOTEFI_HELLO_COMPILE
 	default y if CMD_BOOTEFI_SELFTEST
 	help
 	  This adds a standard EFI hello world application to U-Boot so that
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 34e7fbbf1840..484c9fad239f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -90,11 +90,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
  * back to u-boot world
  */
 void efi_restore_gd(void);
-/* Call this to unset the current device name */
-void efi_clear_bootdev(void);
-/* Call this to set the current device name */
-void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
-		     void *buffer, size_t buffer_size);
+
 /* Called by networking code to memorize the dhcp ack package */
 void efi_net_set_dhcp_ack(void *pkt, int len);
 /* Print information about all loaded images */
@@ -116,10 +112,6 @@ static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
 
 /* No loader configured, stub out EFI_ENTRY */
 static inline void efi_restore_gd(void) { }
-static inline void efi_clear_bootdev(void) { }
-static inline void efi_set_bootdev(const char *dev, const char *devnr,
-				   const char *path, void *buffer,
-				   size_t buffer_size) { }
 static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
 static inline void efi_print_image_infos(void *pc) { }
 static inline efi_status_t efi_launch_capsules(void)
@@ -129,6 +121,20 @@ static inline efi_status_t efi_launch_capsules(void)
 
 #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
 
+#if CONFIG_IS_ENABLED(EFI_BINARY_EXEC)
+/* Call this to unset the current device name */
+void efi_clear_bootdev(void);
+/* Call this to set the current device name */
+void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
+		     void *buffer, size_t buffer_size);
+#else
+static inline void efi_clear_bootdev(void) { }
+
+static inline void efi_set_bootdev(const char *dev, const char *devnr,
+				   const char *path, void *buffer,
+				   size_t buffer_size) { }
+#endif
+
 /* Maximum number of configuration tables */
 #define EFI_MAX_CONFIGURATION_TABLES 16
 
@@ -541,8 +547,8 @@ efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var,
 				      u16 **load_options);
 /* Install device tree */
 efi_status_t efi_install_fdt(void *fdt);
-/* Run loaded UEFI image */
-efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
+/* Execute loaded UEFI image */
+efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options);
 /* Run loaded UEFI image with given fdt */
 efi_status_t efi_binary_run(void *image, size_t size, void *fdt);
 /* Initialize variable services */
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index ea807342f02f..64f2f1cdd161 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -32,6 +32,15 @@ config EFI_LOADER
 
 if EFI_LOADER
 
+config EFI_BINARY_EXEC
+	bool "Execute UEFI binary"
+	default y
+	help
+	  Select this option if you want to execute the UEFI binary after
+	  loading it with U-Boot load commands or other methods.
+	  You may enable CMD_BOOTEFI_BINARY so that you can use bootefi
+	  command to do that.
+
 config BOOTEFI_BOOTMGR
 	bool "UEFI Boot Manager"
 	default y
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 56d97f23827b..e3b27cd7db3e 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -3,8 +3,6 @@
  *  EFI boot manager
  *
  *  Copyright (c) 2017 Rob Clark
- *  For the code moved from cmd/bootefi.c
- *  Copyright (c) 2016 Alexander Graf
  */
 
 #define LOG_CATEGORY LOGC_EFI
@@ -22,17 +20,6 @@
 #include <efi_variable.h>
 #include <asm/unaligned.h>
 
-/* TODO: temporarily added here; clean up later */
-#include <bootm.h>
-#include <efi_selftest.h>
-#include <env.h>
-#include <mapmem.h>
-#include <asm/global_data.h>
-#include <linux/libfdt.h>
-#include <linux/libfdt_env.h>
-
-DECLARE_GLOBAL_DATA_PTR;
-
 static const struct efi_boot_services *bs;
 static const struct efi_runtime_services *rs;
 
@@ -1129,389 +1116,6 @@ out:
 	return ret;
 }
 
-static struct efi_device_path *bootefi_image_path;
-static struct efi_device_path *bootefi_device_path;
-static void *image_addr;
-static size_t image_size;
-
-/**
- * efi_get_image_parameters() - return image parameters
- *
- * @img_addr:		address of loaded image in memory
- * @img_size:		size of loaded image
- */
-void efi_get_image_parameters(void **img_addr, size_t *img_size)
-{
-	*img_addr = image_addr;
-	*img_size = image_size;
-}
-
-/**
- * efi_clear_bootdev() - clear boot device
- */
-void efi_clear_bootdev(void)
-{
-	efi_free_pool(bootefi_device_path);
-	efi_free_pool(bootefi_image_path);
-	bootefi_device_path = NULL;
-	bootefi_image_path = NULL;
-	image_addr = NULL;
-	image_size = 0;
-}
-
-/**
- * efi_set_bootdev() - set boot device
- *
- * This function is called when a file is loaded, e.g. via the 'load' command.
- * We use the path to this file to inform the UEFI binary about the boot device.
- *
- * @dev:		device, e.g. "MMC"
- * @devnr:		number of the device, e.g. "1:2"
- * @path:		path to file loaded
- * @buffer:		buffer with file loaded
- * @buffer_size:	size of file loaded
- */
-void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
-		     void *buffer, size_t buffer_size)
-{
-	struct efi_device_path *device, *image;
-	efi_status_t ret;
-
-	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
-		  devnr, path, buffer, buffer_size);
-
-	/* Forget overwritten image */
-	if (buffer + buffer_size >= image_addr &&
-	    image_addr + image_size >= buffer)
-		efi_clear_bootdev();
-
-	/* Remember only PE-COFF and FIT images */
-	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
-		if (IS_ENABLED(CONFIG_FIT) &&
-		    !fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
-			/*
-			 * FIT images of type EFI_OS are started via command
-			 * bootm. We should not use their boot device with the
-			 * bootefi command.
-			 */
-			buffer = 0;
-			buffer_size = 0;
-		} else {
-			log_debug("- not remembering image\n");
-			return;
-		}
-	}
-
-	/* efi_set_bootdev() is typically called repeatedly, recover memory */
-	efi_clear_bootdev();
-
-	image_addr = buffer;
-	image_size = buffer_size;
-
-	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
-	if (ret == EFI_SUCCESS) {
-		bootefi_device_path = device;
-		if (image) {
-			/* FIXME: image should not contain device */
-			struct efi_device_path *image_tmp = image;
-
-			efi_dp_split_file_path(image, &device, &image);
-			efi_free_pool(image_tmp);
-		}
-		bootefi_image_path = image;
-		log_debug("- boot device %pD\n", device);
-		if (image)
-			log_debug("- image %pD\n", image);
-	} else {
-		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
-		efi_clear_bootdev();
-	}
-}
-
-/**
- * efi_env_set_load_options() - set load options from environment variable
- *
- * @handle:		the image handle
- * @env_var:		name of the environment variable
- * @load_options:	pointer to load options (output)
- * Return:		status code
- */
-efi_status_t efi_env_set_load_options(efi_handle_t handle,
-				      const char *env_var,
-				      u16 **load_options)
-{
-	const char *env = env_get(env_var);
-	size_t size;
-	u16 *pos;
-	efi_status_t ret;
-
-	*load_options = NULL;
-	if (!env)
-		return EFI_SUCCESS;
-	size = sizeof(u16) * (utf8_utf16_strlen(env) + 1);
-	pos = calloc(size, 1);
-	if (!pos)
-		return EFI_OUT_OF_RESOURCES;
-	*load_options = pos;
-	utf8_utf16_strcpy(&pos, env);
-	ret = efi_set_load_options(handle, size, *load_options);
-	if (ret != EFI_SUCCESS) {
-		free(*load_options);
-		*load_options = NULL;
-	}
-	return ret;
-}
-
-#if !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
-
-/**
- * copy_fdt() - Copy the device tree to a new location available to EFI
- *
- * The FDT is copied to a suitable location within the EFI memory map.
- * Additional 12 KiB are added to the space in case the device tree needs to be
- * expanded later with fdt_open_into().
- *
- * @fdtp:	On entry a pointer to the flattened device tree.
- *		On exit a pointer to the copy of the flattened device tree.
- *		FDT start
- * Return:	status code
- */
-static efi_status_t copy_fdt(void **fdtp)
-{
-	unsigned long fdt_ram_start = -1L, fdt_pages;
-	efi_status_t ret = 0;
-	void *fdt, *new_fdt;
-	u64 new_fdt_addr;
-	uint fdt_size;
-	int i;
-
-	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
-		u64 ram_start = gd->bd->bi_dram[i].start;
-		u64 ram_size = gd->bd->bi_dram[i].size;
-
-		if (!ram_size)
-			continue;
-
-		if (ram_start < fdt_ram_start)
-			fdt_ram_start = ram_start;
-	}
-
-	/*
-	 * Give us at least 12 KiB of breathing room in case the device tree
-	 * needs to be expanded later.
-	 */
-	fdt = *fdtp;
-	fdt_pages = efi_size_in_pages(fdt_totalsize(fdt) + 0x3000);
-	fdt_size = fdt_pages << EFI_PAGE_SHIFT;
-
-	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
-				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
-				 &new_fdt_addr);
-	if (ret != EFI_SUCCESS) {
-		log_err("ERROR: Failed to reserve space for FDT\n");
-		goto done;
-	}
-	new_fdt = (void *)(uintptr_t)new_fdt_addr;
-	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
-	fdt_set_totalsize(new_fdt, fdt_size);
-
-	*fdtp = (void *)(uintptr_t)new_fdt_addr;
-done:
-	return ret;
-}
-
-/**
- * get_config_table() - get configuration table
- *
- * @guid:	GUID of the configuration table
- * Return:	pointer to configuration table or NULL
- */
-static void *get_config_table(const efi_guid_t *guid)
-{
-	size_t i;
-
-	for (i = 0; i < systab.nr_tables; i++) {
-		if (!guidcmp(guid, &systab.tables[i].guid))
-			return systab.tables[i].table;
-	}
-	return NULL;
-}
-
-#endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
-
-/**
- * efi_install_fdt() - install device tree
- *
- * If fdt is not EFI_FDT_USE_INTERNAL, the device tree located at that memory
- * address will be installed as configuration table, otherwise the device
- * tree located at the address indicated by environment variable fdt_addr or as
- * fallback fdtcontroladdr will be used.
- *
- * On architectures using ACPI tables device trees shall not be installed as
- * configuration table.
- *
- * @fdt:	address of device tree or EFI_FDT_USE_INTERNAL to use
- *		the hardware device tree as indicated by environment variable
- *		fdt_addr or as fallback the internal device tree as indicated by
- *		the environment variable fdtcontroladdr
- * Return:	status code
- */
-efi_status_t efi_install_fdt(void *fdt)
-{
-	/*
-	 * The EBBR spec requires that we have either an FDT or an ACPI table
-	 * but not both.
-	 */
-#if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
-	if (fdt) {
-		log_warning("WARNING: Can't have ACPI table and device tree - ignoring DT.\n");
-		return EFI_SUCCESS;
-	}
-#else
-	struct bootm_headers img = { 0 };
-	efi_status_t ret;
-
-	if (fdt == EFI_FDT_USE_INTERNAL) {
-		const char *fdt_opt;
-		uintptr_t fdt_addr;
-
-		/* Look for device tree that is already installed */
-		if (get_config_table(&efi_guid_fdt))
-			return EFI_SUCCESS;
-		/* Check if there is a hardware device tree */
-		fdt_opt = env_get("fdt_addr");
-		/* Use our own device tree as fallback */
-		if (!fdt_opt) {
-			fdt_opt = env_get("fdtcontroladdr");
-			if (!fdt_opt) {
-				log_err("ERROR: need device tree\n");
-				return EFI_NOT_FOUND;
-			}
-		}
-		fdt_addr = hextoul(fdt_opt, NULL);
-		if (!fdt_addr) {
-			log_err("ERROR: invalid $fdt_addr or $fdtcontroladdr\n");
-			return EFI_LOAD_ERROR;
-		}
-		fdt = map_sysmem(fdt_addr, 0);
-	}
-
-	/* Install device tree */
-	if (fdt_check_header(fdt)) {
-		log_err("ERROR: invalid device tree\n");
-		return EFI_LOAD_ERROR;
-	}
-
-	/* Prepare device tree for payload */
-	ret = copy_fdt(&fdt);
-	if (ret) {
-		log_err("ERROR: out of memory\n");
-		return EFI_OUT_OF_RESOURCES;
-	}
-
-	if (image_setup_libfdt(&img, fdt, NULL)) {
-		log_err("ERROR: failed to process device tree\n");
-		return EFI_LOAD_ERROR;
-	}
-
-	/* Create memory reservations as indicated by the device tree */
-	efi_carve_out_dt_rsv(fdt);
-
-	efi_try_purge_kaslr_seed(fdt);
-
-	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
-		ret = efi_tcg2_measure_dtb(fdt);
-		if (ret == EFI_SECURITY_VIOLATION) {
-			log_err("ERROR: failed to measure DTB\n");
-			return ret;
-		}
-	}
-
-	/* Install device tree as UEFI table */
-	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
-	if (ret != EFI_SUCCESS) {
-		log_err("ERROR: failed to install device tree\n");
-		return ret;
-	}
-#endif /* GENERATE_ACPI_TABLE */
-
-	return EFI_SUCCESS;
-}
-
-/**
- * do_bootefi_exec() - execute EFI binary
- *
- * The image indicated by @handle is started. When it returns the allocated
- * memory for the @load_options is freed.
- *
- * @handle:		handle of loaded image
- * @load_options:	load options
- * Return:		status code
- *
- * Load the EFI binary into a newly assigned memory unwinding the relocation
- * information, install the loaded image protocol, and call the binary.
- */
-static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
-{
-	efi_status_t ret;
-	efi_uintn_t exit_data_size = 0;
-	u16 *exit_data = NULL;
-	struct efi_event *evt;
-
-	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
-	switch_to_non_secure_mode();
-
-	/*
-	 * The UEFI standard requires that the watchdog timer is set to five
-	 * minutes when invoking an EFI boot option.
-	 *
-	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
-	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
-	 */
-	ret = efi_set_watchdog(300);
-	if (ret != EFI_SUCCESS) {
-		log_err("ERROR: Failed to set watchdog timer\n");
-		goto out;
-	}
-
-	/* Call our payload! */
-	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
-	if (ret != EFI_SUCCESS) {
-		log_err("## Application failed, r = %lu\n",
-			ret & ~EFI_ERROR_MASK);
-		if (exit_data) {
-			log_err("## %ls\n", exit_data);
-			efi_free_pool(exit_data);
-		}
-	}
-
-	efi_restore_gd();
-
-out:
-	free(load_options);
-
-	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
-		if (efi_initrd_deregister() != EFI_SUCCESS)
-			log_err("Failed to remove loadfile2 for initrd\n");
-	}
-
-	/* Notify EFI_EVENT_GROUP_RETURN_TO_EFIBOOTMGR event group. */
-	list_for_each_entry(evt, &efi_events, link) {
-		if (evt->group &&
-		    !guidcmp(evt->group,
-			     &efi_guid_event_group_return_to_efibootmgr)) {
-			efi_signal_event(evt);
-			EFI_CALL(systab.boottime->close_event(evt));
-			break;
-		}
-	}
-
-	/* Control is returned to U-Boot, disable EFI watchdog */
-	efi_set_watchdog(0);
-
-	return ret;
-}
-
 /**
  * efi_bootmgr_run() - execute EFI boot manager
  * @fdt:	Flat device tree
@@ -1548,100 +1152,3 @@ efi_status_t efi_bootmgr_run(void *fdt)
 
 	return do_bootefi_exec(handle, load_options);
 }
-
-/**
- * efi_run_image() - run loaded UEFI image
- *
- * @source_buffer:	memory address of the UEFI image
- * @source_size:	size of the UEFI image
- * Return:		status code
- */
-efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
-{
-	efi_handle_t mem_handle = NULL, handle;
-	struct efi_device_path *file_path = NULL;
-	struct efi_device_path *msg_path;
-	efi_status_t ret, ret2;
-	u16 *load_options;
-
-	if (!bootefi_device_path || !bootefi_image_path) {
-		log_debug("Not loaded from disk\n");
-		/*
-		 * Special case for efi payload not loaded from disk,
-		 * such as 'bootefi hello' or for example payload
-		 * loaded directly into memory via JTAG, etc:
-		 */
-		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
-					    (uintptr_t)source_buffer,
-					    source_size);
-		/*
-		 * Make sure that device for device_path exist
-		 * in load_image(). Otherwise, shell and grub will fail.
-		 */
-		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
-							       &efi_guid_device_path,
-							       file_path, NULL);
-		if (ret != EFI_SUCCESS)
-			goto out;
-		msg_path = file_path;
-	} else {
-		file_path = efi_dp_append(bootefi_device_path,
-					  bootefi_image_path);
-		msg_path = bootefi_image_path;
-		log_debug("Loaded from disk\n");
-	}
-
-	log_info("Booting %pD\n", msg_path);
-
-	ret = EFI_CALL(efi_load_image(false, efi_root, file_path, source_buffer,
-				      source_size, &handle));
-	if (ret != EFI_SUCCESS) {
-		log_err("Loading image failed\n");
-		goto out;
-	}
-
-	/* Transfer environment variable as load options */
-	ret = efi_env_set_load_options(handle, "bootargs", &load_options);
-	if (ret != EFI_SUCCESS)
-		goto out;
-
-	ret = do_bootefi_exec(handle, load_options);
-
-out:
-	ret2 = efi_uninstall_multiple_protocol_interfaces(mem_handle,
-							  &efi_guid_device_path,
-							  file_path, NULL);
-	efi_free_pool(file_path);
-	return (ret != EFI_SUCCESS) ? ret : ret2;
-}
-
-/**
- * efi_binary_run() - run loaded UEFI image
- *
- * @image:	memory address of the UEFI image
- * @size:	size of the UEFI image
- * @fdt:	device-tree
- *
- * Execute an EFI binary image loaded at @image.
- * @size may be zero if the binary is loaded with U-Boot load command.
- *
- * Return:	status code
- */
-efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
-{
-	efi_status_t ret;
-
-	/* Initialize EFI drivers */
-	ret = efi_init_obj_list();
-	if (ret != EFI_SUCCESS) {
-		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
-			ret & ~EFI_ERROR_MASK);
-		return -1;
-	}
-
-	ret = efi_install_fdt(fdt);
-	if (ret != EFI_SUCCESS)
-		return ret;
-
-	return efi_run_image(image, size);
-}
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index ed7214f3a347..786d8a70e2ad 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -1090,7 +1090,8 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 	if (path && !file)
 		return EFI_INVALID_PARAMETER;
 
-	if (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs"))  {
+	if (IS_ENABLED(CONFIG_EFI_BINARY_EXEC) &&
+	    (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs")))  {
 		/* loadm command and semihosting */
 		efi_get_image_parameters(&image_addr, &image_size);
 
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index cdfd16ea7742..79a2a579e901 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -1,17 +1,28 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
  * Copyright (c) 2020, Linaro Limited
+ * For the code moved from cmd/bootefi.c
+ * Copyright (c) 2016 Alexander Graf
  */
 
 #define LOG_CATEGORY LOGC_EFI
+#include <bootm.h>
 #include <common.h>
-#include <env.h>
-#include <malloc.h>
 #include <dm.h>
-#include <fs.h>
 #include <efi_load_initrd.h>
 #include <efi_loader.h>
 #include <efi_variable.h>
+#include <env.h>
+#include <fs.h>
+#include <log.h>
+#include <malloc.h>
+#include <mapmem.h>
+#include <vsprintf.h>
+#include <asm/global_data.h>
+#include <linux/libfdt.h>
+#include <linux/libfdt_env.h>
+
+DECLARE_GLOBAL_DATA_PTR;
 
 #if defined(CONFIG_CMD_EFIDEBUG) || defined(CONFIG_EFI_LOAD_FILE2_INITRD)
 /* GUID used by Linux to identify the LoadFile2 protocol with the initrd */
@@ -282,3 +293,485 @@ bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *inde
 
 	return false;
 }
+
+#if !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
+
+/**
+ * copy_fdt() - Copy the device tree to a new location available to EFI
+ *
+ * The FDT is copied to a suitable location within the EFI memory map.
+ * Additional 12 KiB are added to the space in case the device tree needs to be
+ * expanded later with fdt_open_into().
+ *
+ * @fdtp:	On entry a pointer to the flattened device tree.
+ *		On exit a pointer to the copy of the flattened device tree.
+ *		FDT start
+ * Return:	status code
+ */
+static efi_status_t copy_fdt(void **fdtp)
+{
+	unsigned long fdt_ram_start = -1L, fdt_pages;
+	efi_status_t ret = 0;
+	void *fdt, *new_fdt;
+	u64 new_fdt_addr;
+	uint fdt_size;
+	int i;
+
+	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
+		u64 ram_start = gd->bd->bi_dram[i].start;
+		u64 ram_size = gd->bd->bi_dram[i].size;
+
+		if (!ram_size)
+			continue;
+
+		if (ram_start < fdt_ram_start)
+			fdt_ram_start = ram_start;
+	}
+
+	/*
+	 * Give us at least 12 KiB of breathing room in case the device tree
+	 * needs to be expanded later.
+	 */
+	fdt = *fdtp;
+	fdt_pages = efi_size_in_pages(fdt_totalsize(fdt) + 0x3000);
+	fdt_size = fdt_pages << EFI_PAGE_SHIFT;
+
+	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
+				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
+				 &new_fdt_addr);
+	if (ret != EFI_SUCCESS) {
+		log_err("ERROR: Failed to reserve space for FDT\n");
+		goto done;
+	}
+	new_fdt = (void *)(uintptr_t)new_fdt_addr;
+	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
+	fdt_set_totalsize(new_fdt, fdt_size);
+
+	*fdtp = (void *)(uintptr_t)new_fdt_addr;
+done:
+	return ret;
+}
+
+/**
+ * get_config_table() - get configuration table
+ *
+ * @guid:	GUID of the configuration table
+ * Return:	pointer to configuration table or NULL
+ */
+static void *get_config_table(const efi_guid_t *guid)
+{
+	size_t i;
+
+	for (i = 0; i < systab.nr_tables; i++) {
+		if (!guidcmp(guid, &systab.tables[i].guid))
+			return systab.tables[i].table;
+	}
+	return NULL;
+}
+
+#endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
+
+/**
+ * efi_install_fdt() - install device tree
+ *
+ * If fdt is not EFI_FDT_USE_INTERNAL, the device tree located at that memory
+ * address will be installed as configuration table, otherwise the device
+ * tree located at the address indicated by environment variable fdt_addr or as
+ * fallback fdtcontroladdr will be used.
+ *
+ * On architectures using ACPI tables device trees shall not be installed as
+ * configuration table.
+ *
+ * @fdt:	address of device tree or EFI_FDT_USE_INTERNAL to use
+ *		the hardware device tree as indicated by environment variable
+ *		fdt_addr or as fallback the internal device tree as indicated by
+ *		the environment variable fdtcontroladdr
+ * Return:	status code
+ */
+efi_status_t efi_install_fdt(void *fdt)
+{
+	/*
+	 * The EBBR spec requires that we have either an FDT or an ACPI table
+	 * but not both.
+	 */
+#if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
+	if (fdt) {
+		log_warning("WARNING: Can't have ACPI table and device tree - ignoring DT.\n");
+		return EFI_SUCCESS;
+	}
+#else
+	struct bootm_headers img = { 0 };
+	efi_status_t ret;
+
+	if (fdt == EFI_FDT_USE_INTERNAL) {
+		const char *fdt_opt;
+		uintptr_t fdt_addr;
+
+		/* Look for device tree that is already installed */
+		if (get_config_table(&efi_guid_fdt))
+			return EFI_SUCCESS;
+		/* Check if there is a hardware device tree */
+		fdt_opt = env_get("fdt_addr");
+		/* Use our own device tree as fallback */
+		if (!fdt_opt) {
+			fdt_opt = env_get("fdtcontroladdr");
+			if (!fdt_opt) {
+				log_err("ERROR: need device tree\n");
+				return EFI_NOT_FOUND;
+			}
+		}
+		fdt_addr = hextoul(fdt_opt, NULL);
+		if (!fdt_addr) {
+			log_err("ERROR: invalid $fdt_addr or $fdtcontroladdr\n");
+			return EFI_LOAD_ERROR;
+		}
+		fdt = map_sysmem(fdt_addr, 0);
+	}
+
+	/* Install device tree */
+	if (fdt_check_header(fdt)) {
+		log_err("ERROR: invalid device tree\n");
+		return EFI_LOAD_ERROR;
+	}
+
+	/* Prepare device tree for payload */
+	ret = copy_fdt(&fdt);
+	if (ret) {
+		log_err("ERROR: out of memory\n");
+		return EFI_OUT_OF_RESOURCES;
+	}
+
+	if (image_setup_libfdt(&img, fdt, NULL)) {
+		log_err("ERROR: failed to process device tree\n");
+		return EFI_LOAD_ERROR;
+	}
+
+	/* Create memory reservations as indicated by the device tree */
+	efi_carve_out_dt_rsv(fdt);
+
+	efi_try_purge_kaslr_seed(fdt);
+
+	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
+		ret = efi_tcg2_measure_dtb(fdt);
+		if (ret == EFI_SECURITY_VIOLATION) {
+			log_err("ERROR: failed to measure DTB\n");
+			return ret;
+		}
+	}
+
+	/* Install device tree as UEFI table */
+	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
+	if (ret != EFI_SUCCESS) {
+		log_err("ERROR: failed to install device tree\n");
+		return ret;
+	}
+#endif /* GENERATE_ACPI_TABLE */
+
+	return EFI_SUCCESS;
+}
+
+/**
+ * do_bootefi_exec() - execute EFI binary
+ *
+ * The image indicated by @handle is started. When it returns the allocated
+ * memory for the @load_options is freed.
+ *
+ * @handle:		handle of loaded image
+ * @load_options:	load options
+ * Return:		status code
+ *
+ * Load the EFI binary into a newly assigned memory unwinding the relocation
+ * information, install the loaded image protocol, and call the binary.
+ */
+efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
+{
+	efi_status_t ret;
+	efi_uintn_t exit_data_size = 0;
+	u16 *exit_data = NULL;
+	struct efi_event *evt;
+
+	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
+	switch_to_non_secure_mode();
+
+	/*
+	 * The UEFI standard requires that the watchdog timer is set to five
+	 * minutes when invoking an EFI boot option.
+	 *
+	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
+	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
+	 */
+	ret = efi_set_watchdog(300);
+	if (ret != EFI_SUCCESS) {
+		log_err("ERROR: Failed to set watchdog timer\n");
+		goto out;
+	}
+
+	/* Call our payload! */
+	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
+	if (ret != EFI_SUCCESS) {
+		log_err("## Application failed, r = %lu\n",
+			ret & ~EFI_ERROR_MASK);
+		if (exit_data) {
+			log_err("## %ls\n", exit_data);
+			efi_free_pool(exit_data);
+		}
+	}
+
+	efi_restore_gd();
+
+out:
+	free(load_options);
+
+	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
+		if (efi_initrd_deregister() != EFI_SUCCESS)
+			log_err("Failed to remove loadfile2 for initrd\n");
+	}
+
+	/* Notify EFI_EVENT_GROUP_RETURN_TO_EFIBOOTMGR event group. */
+	list_for_each_entry(evt, &efi_events, link) {
+		if (evt->group &&
+		    !guidcmp(evt->group,
+			     &efi_guid_event_group_return_to_efibootmgr)) {
+			efi_signal_event(evt);
+			EFI_CALL(systab.boottime->close_event(evt));
+			break;
+		}
+	}
+
+	/* Control is returned to U-Boot, disable EFI watchdog */
+	efi_set_watchdog(0);
+
+	return ret;
+}
+
+#if CONFIG_IS_ENABLED(EFI_BINARY_EXEC)
+static struct efi_device_path *bootefi_image_path;
+static struct efi_device_path *bootefi_device_path;
+static void *image_addr;
+static size_t image_size;
+
+/**
+ * efi_get_image_parameters() - return image parameters
+ *
+ * @img_addr:		address of loaded image in memory
+ * @img_size:		size of loaded image
+ */
+void efi_get_image_parameters(void **img_addr, size_t *img_size)
+{
+	*img_addr = image_addr;
+	*img_size = image_size;
+}
+
+/**
+ * efi_clear_bootdev() - clear boot device
+ */
+void efi_clear_bootdev(void)
+{
+	efi_free_pool(bootefi_device_path);
+	efi_free_pool(bootefi_image_path);
+	bootefi_device_path = NULL;
+	bootefi_image_path = NULL;
+	image_addr = NULL;
+	image_size = 0;
+}
+
+/**
+ * efi_set_bootdev() - set boot device
+ *
+ * This function is called when a file is loaded, e.g. via the 'load' command.
+ * We use the path to this file to inform the UEFI binary about the boot device.
+ *
+ * @dev:		device, e.g. "MMC"
+ * @devnr:		number of the device, e.g. "1:2"
+ * @path:		path to file loaded
+ * @buffer:		buffer with file loaded
+ * @buffer_size:	size of file loaded
+ */
+void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
+		     void *buffer, size_t buffer_size)
+{
+	struct efi_device_path *device, *image;
+	efi_status_t ret;
+
+	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
+		  devnr, path, buffer, buffer_size);
+
+	/* Forget overwritten image */
+	if (buffer + buffer_size >= image_addr &&
+	    image_addr + image_size >= buffer)
+		efi_clear_bootdev();
+
+	/* Remember only PE-COFF and FIT images */
+	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
+		if (IS_ENABLED(CONFIG_FIT) &&
+		    !fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
+			/*
+			 * FIT images of type EFI_OS are started via command
+			 * bootm. We should not use their boot device with the
+			 * bootefi command.
+			 */
+			buffer = 0;
+			buffer_size = 0;
+		} else {
+			log_debug("- not remembering image\n");
+			return;
+		}
+	}
+
+	/* efi_set_bootdev() is typically called repeatedly, recover memory */
+	efi_clear_bootdev();
+
+	image_addr = buffer;
+	image_size = buffer_size;
+
+	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
+	if (ret == EFI_SUCCESS) {
+		bootefi_device_path = device;
+		if (image) {
+			/* FIXME: image should not contain device */
+			struct efi_device_path *image_tmp = image;
+
+			efi_dp_split_file_path(image, &device, &image);
+			efi_free_pool(image_tmp);
+		}
+		bootefi_image_path = image;
+		log_debug("- boot device %pD\n", device);
+		if (image)
+			log_debug("- image %pD\n", image);
+	} else {
+		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
+		efi_clear_bootdev();
+	}
+}
+
+/**
+ * efi_env_set_load_options() - set load options from environment variable
+ *
+ * @handle:		the image handle
+ * @env_var:		name of the environment variable
+ * @load_options:	pointer to load options (output)
+ * Return:		status code
+ */
+efi_status_t efi_env_set_load_options(efi_handle_t handle,
+				      const char *env_var,
+				      u16 **load_options)
+{
+	const char *env = env_get(env_var);
+	size_t size;
+	u16 *pos;
+	efi_status_t ret;
+
+	*load_options = NULL;
+	if (!env)
+		return EFI_SUCCESS;
+	size = sizeof(u16) * (utf8_utf16_strlen(env) + 1);
+	pos = calloc(size, 1);
+	if (!pos)
+		return EFI_OUT_OF_RESOURCES;
+	*load_options = pos;
+	utf8_utf16_strcpy(&pos, env);
+	ret = efi_set_load_options(handle, size, *load_options);
+	if (ret != EFI_SUCCESS) {
+		free(*load_options);
+		*load_options = NULL;
+	}
+	return ret;
+}
+
+/**
+ * efi_run_image() - run loaded UEFI image
+ *
+ * @source_buffer:	memory address of the UEFI image
+ * @source_size:	size of the UEFI image
+ * Return:		status code
+ */
+efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
+{
+	efi_handle_t mem_handle = NULL, handle;
+	struct efi_device_path *file_path = NULL;
+	struct efi_device_path *msg_path;
+	efi_status_t ret, ret2;
+	u16 *load_options;
+
+	if (!bootefi_device_path || !bootefi_image_path) {
+		log_debug("Not loaded from disk\n");
+		/*
+		 * Special case for efi payload not loaded from disk,
+		 * such as 'bootefi hello' or for example payload
+		 * loaded directly into memory via JTAG, etc:
+		 */
+		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+					    (uintptr_t)source_buffer,
+					    source_size);
+		/*
+		 * Make sure that device for device_path exist
+		 * in load_image(). Otherwise, shell and grub will fail.
+		 */
+		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
+							       &efi_guid_device_path,
+							       file_path, NULL);
+		if (ret != EFI_SUCCESS)
+			goto out;
+		msg_path = file_path;
+	} else {
+		file_path = efi_dp_append(bootefi_device_path,
+					  bootefi_image_path);
+		msg_path = bootefi_image_path;
+		log_debug("Loaded from disk\n");
+	}
+
+	log_info("Booting %pD\n", msg_path);
+
+	ret = EFI_CALL(efi_load_image(false, efi_root, file_path, source_buffer,
+				      source_size, &handle));
+	if (ret != EFI_SUCCESS) {
+		log_err("Loading image failed\n");
+		goto out;
+	}
+
+	/* Transfer environment variable as load options */
+	ret = efi_env_set_load_options(handle, "bootargs", &load_options);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = do_bootefi_exec(handle, load_options);
+
+out:
+	ret2 = efi_uninstall_multiple_protocol_interfaces(mem_handle,
+							  &efi_guid_device_path,
+							  file_path, NULL);
+	efi_free_pool(file_path);
+	return (ret != EFI_SUCCESS) ? ret : ret2;
+}
+
+/**
+ * efi_binary_run() - run loaded UEFI image
+ *
+ * @image:	memory address of the UEFI image
+ * @size:	size of the UEFI image
+ * @fdt:	device-tree
+ *
+ * Execute an EFI binary image loaded at @image.
+ * @size may be zero if the binary is loaded with U-Boot load command.
+ *
+ * Return:	status code
+ */
+efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
+{
+	efi_status_t ret;
+
+	/* Initialize EFI drivers */
+	ret = efi_init_obj_list();
+	if (ret != EFI_SUCCESS) {
+		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+			ret & ~EFI_ERROR_MASK);
+		return -1;
+	}
+
+	ret = efi_install_fdt(fdt);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	return efi_run_image(image, size);
+}
+#endif /* CONFIG_BINARY_EXEC */
-- 
2.34.1

Re: [PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c

[Simon Glass]

Hi AKASHI,

On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Some code moved from cmd/bootefi.c is actually necessary only for "bootefi
> <addr>" command (starting an image manually loaded by a user using U-Boot
> load commands or other methods (like JTAG debugger).
>
> The code will never been opted out as unused code by a compiler which
> doesn't know how EFI boot manager is implemented. So introduce a new
> configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out
> explicitly.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  boot/Kconfig                     |   4 +-
>  cmd/Kconfig                      |   6 +-
>  include/efi_loader.h             |  28 +-
>  lib/efi_loader/Kconfig           |   9 +
>  lib/efi_loader/efi_bootmgr.c     | 493 ------------------------------
>  lib/efi_loader/efi_device_path.c |   3 +-
>  lib/efi_loader/efi_helper.c      | 499 ++++++++++++++++++++++++++++++-
>  7 files changed, 529 insertions(+), 513 deletions(-)

'helper' seems a bit vague to me. How about efi_boot.c ?

REgards,
Simon

Re: [PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c

[AKASHI Takahiro]

On Mon, Dec 18, 2023 at 08:01:51AM -0700, Simon Glass wrote:
> Hi AKASHI,
> 
> On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Some code moved from cmd/bootefi.c is actually necessary only for "bootefi
> > <addr>" command (starting an image manually loaded by a user using U-Boot
> > load commands or other methods (like JTAG debugger).
> >
> > The code will never been opted out as unused code by a compiler which
> > doesn't know how EFI boot manager is implemented. So introduce a new
> > configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out
> > explicitly.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  boot/Kconfig                     |   4 +-
> >  cmd/Kconfig                      |   6 +-
> >  include/efi_loader.h             |  28 +-
> >  lib/efi_loader/Kconfig           |   9 +
> >  lib/efi_loader/efi_bootmgr.c     | 493 ------------------------------
> >  lib/efi_loader/efi_device_path.c |   3 +-
> >  lib/efi_loader/efi_helper.c      | 499 ++++++++++++++++++++++++++++++-
> >  7 files changed, 529 insertions(+), 513 deletions(-)
> 
> 'helper' seems a bit vague to me. How about efi_boot.c ?

Although I hesitated to add one more new file as we already have
efi_boottime.c and efi_bootmgr.c, then efi_boot.c?, okay I will do that.

-Takahiro Akashi


> REgards,
> Simon

Re: [PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c

[Heinrich Schuchardt]

On 12/18/23 03:38, AKASHI Takahiro wrote:
> Some code moved from cmd/bootefi.c is actually necessary only for "bootefi
> <addr>" command (starting an image manually loaded by a user using U-Boot
> load commands or other methods (like JTAG debugger).
>
> The code will never been opted out as unused code by a compiler which
> doesn't know how EFI boot manager is implemented. So introduce a new
> configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out
> explicitly.

We build with -ffunction-sections. The linker removes unreferenced
functions.

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   boot/Kconfig                     |   4 +-
>   cmd/Kconfig                      |   6 +-
>   include/efi_loader.h             |  28 +-
>   lib/efi_loader/Kconfig           |   9 +
>   lib/efi_loader/efi_bootmgr.c     | 493 ------------------------------
>   lib/efi_loader/efi_device_path.c |   3 +-
>   lib/efi_loader/efi_helper.c      | 499 ++++++++++++++++++++++++++++++-

We expect that after each patch we can compile the code. This requires
that the Makefile change is in the same patch as the creation of
efi_helper.c.

>   7 files changed, 529 insertions(+), 513 deletions(-)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index 987ca7314117..8ab7e6f63d34 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -523,7 +523,7 @@ config BOOTMETH_EXTLINUX_PXE
>
>   config BOOTMETH_EFILOADER
>   	bool "Bootdev support for EFI boot"
> -	depends on BOOTEFI_BOOTMGR
> +	depends on EFI_BINARY_EXEC

Why do we need a symbol CONFIG_EFI_BINARY_EXEC? CONFIG_EFI_LOADER=y
without the ability to execute an EFI binary makes no sense to me.

Best regards

Heinrich

>   	default y
>   	help
>   	  Enables support for EFI boot using bootdevs. This makes the
> @@ -558,7 +558,7 @@ config BOOTMETH_DISTRO
>   	select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
>   	select BOOTMETH_EXTLINUX  # E.g. Debian uses these
>   	select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
> -	select BOOTMETH_EFILOADER if BOOTEFI_BOOTMGR # E.g. Ubuntu uses this
> +	select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
>
>   config SPL_BOOTMETH_VBE
>   	bool "Bootdev support for Verified Boot for Embedded (SPL)"
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 24bfbe505722..2c993496b70e 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -273,7 +273,7 @@ config CMD_BOOTMETH
>
>   config BOOTM_EFI
>   	bool "Support booting UEFI FIT images"
> -	depends on BOOTEFI_BOOTMGR && CMD_BOOTM && FIT
> +	depends on EFI_BINARY_EXEC && CMD_BOOTM && FIT
>   	default y
>   	help
>   	  Support booting UEFI FIT images via the bootm command.
> @@ -365,7 +365,7 @@ config CMD_BOOTEFI
>   if CMD_BOOTEFI
>   config CMD_BOOTEFI_BINARY
>   	bool "Allow booting an EFI binary directly"
> -	depends on BOOTEFI_BOOTMGR
> +	depends on EFI_BINARY_EXEC
>   	default y
>   	help
>   	  Select this option to enable direct execution of binary at 'bootefi'.
> @@ -395,7 +395,7 @@ config CMD_BOOTEFI_HELLO_COMPILE
>
>   config CMD_BOOTEFI_HELLO
>   	bool "Allow booting a standard EFI hello world for testing"
> -	depends on CMD_BOOTEFI_HELLO_COMPILE
> +	depends on CMD_BOOTEFI_BINARY && CMD_BOOTEFI_HELLO_COMPILE
>   	default y if CMD_BOOTEFI_SELFTEST
>   	help
>   	  This adds a standard EFI hello world application to U-Boot so that
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 34e7fbbf1840..484c9fad239f 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -90,11 +90,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
>    * back to u-boot world
>    */
>   void efi_restore_gd(void);
> -/* Call this to unset the current device name */
> -void efi_clear_bootdev(void);
> -/* Call this to set the current device name */
> -void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
> -		     void *buffer, size_t buffer_size);
> +
>   /* Called by networking code to memorize the dhcp ack package */
>   void efi_net_set_dhcp_ack(void *pkt, int len);
>   /* Print information about all loaded images */
> @@ -116,10 +112,6 @@ static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
>
>   /* No loader configured, stub out EFI_ENTRY */
>   static inline void efi_restore_gd(void) { }
> -static inline void efi_clear_bootdev(void) { }
> -static inline void efi_set_bootdev(const char *dev, const char *devnr,
> -				   const char *path, void *buffer,
> -				   size_t buffer_size) { }
>   static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
>   static inline void efi_print_image_infos(void *pc) { }
>   static inline efi_status_t efi_launch_capsules(void)
> @@ -129,6 +121,20 @@ static inline efi_status_t efi_launch_capsules(void)
>
>   #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
>
> +#if CONFIG_IS_ENABLED(EFI_BINARY_EXEC)
> +/* Call this to unset the current device name */
> +void efi_clear_bootdev(void);
> +/* Call this to set the current device name */
> +void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
> +		     void *buffer, size_t buffer_size);
> +#else
> +static inline void efi_clear_bootdev(void) { }
> +
> +static inline void efi_set_bootdev(const char *dev, const char *devnr,
> +				   const char *path, void *buffer,
> +				   size_t buffer_size) { }
> +#endif
> +
>   /* Maximum number of configuration tables */
>   #define EFI_MAX_CONFIGURATION_TABLES 16
>
> @@ -541,8 +547,8 @@ efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var,
>   				      u16 **load_options);
>   /* Install device tree */
>   efi_status_t efi_install_fdt(void *fdt);
> -/* Run loaded UEFI image */
> -efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
> +/* Execute loaded UEFI image */
> +efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options);
>   /* Run loaded UEFI image with given fdt */
>   efi_status_t efi_binary_run(void *image, size_t size, void *fdt);
>   /* Initialize variable services */
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index ea807342f02f..64f2f1cdd161 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -32,6 +32,15 @@ config EFI_LOADER
>
>   if EFI_LOADER
>
> +config EFI_BINARY_EXEC
> +	bool "Execute UEFI binary"
> +	default y
> +	help
> +	  Select this option if you want to execute the UEFI binary after
> +	  loading it with U-Boot load commands or other methods.
> +	  You may enable CMD_BOOTEFI_BINARY so that you can use bootefi
> +	  command to do that.
> +
>   config BOOTEFI_BOOTMGR
>   	bool "UEFI Boot Manager"
>   	default y
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 56d97f23827b..e3b27cd7db3e 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -3,8 +3,6 @@
>    *  EFI boot manager
>    *
>    *  Copyright (c) 2017 Rob Clark
> - *  For the code moved from cmd/bootefi.c
> - *  Copyright (c) 2016 Alexander Graf
>    */
>
>   #define LOG_CATEGORY LOGC_EFI
> @@ -22,17 +20,6 @@
>   #include <efi_variable.h>
>   #include <asm/unaligned.h>
>
> -/* TODO: temporarily added here; clean up later */
> -#include <bootm.h>
> -#include <efi_selftest.h>
> -#include <env.h>
> -#include <mapmem.h>
> -#include <asm/global_data.h>
> -#include <linux/libfdt.h>
> -#include <linux/libfdt_env.h>
> -
> -DECLARE_GLOBAL_DATA_PTR;
> -
>   static const struct efi_boot_services *bs;
>   static const struct efi_runtime_services *rs;
>
> @@ -1129,389 +1116,6 @@ out:
>   	return ret;
>   }
>
> -static struct efi_device_path *bootefi_image_path;
> -static struct efi_device_path *bootefi_device_path;
> -static void *image_addr;
> -static size_t image_size;
> -
> -/**
> - * efi_get_image_parameters() - return image parameters
> - *
> - * @img_addr:		address of loaded image in memory
> - * @img_size:		size of loaded image
> - */
> -void efi_get_image_parameters(void **img_addr, size_t *img_size)
> -{
> -	*img_addr = image_addr;
> -	*img_size = image_size;
> -}
> -
> -/**
> - * efi_clear_bootdev() - clear boot device
> - */
> -void efi_clear_bootdev(void)
> -{
> -	efi_free_pool(bootefi_device_path);
> -	efi_free_pool(bootefi_image_path);
> -	bootefi_device_path = NULL;
> -	bootefi_image_path = NULL;
> -	image_addr = NULL;
> -	image_size = 0;
> -}
> -
> -/**
> - * efi_set_bootdev() - set boot device
> - *
> - * This function is called when a file is loaded, e.g. via the 'load' command.
> - * We use the path to this file to inform the UEFI binary about the boot device.
> - *
> - * @dev:		device, e.g. "MMC"
> - * @devnr:		number of the device, e.g. "1:2"
> - * @path:		path to file loaded
> - * @buffer:		buffer with file loaded
> - * @buffer_size:	size of file loaded
> - */
> -void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
> -		     void *buffer, size_t buffer_size)
> -{
> -	struct efi_device_path *device, *image;
> -	efi_status_t ret;
> -
> -	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
> -		  devnr, path, buffer, buffer_size);
> -
> -	/* Forget overwritten image */
> -	if (buffer + buffer_size >= image_addr &&
> -	    image_addr + image_size >= buffer)
> -		efi_clear_bootdev();
> -
> -	/* Remember only PE-COFF and FIT images */
> -	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
> -		if (IS_ENABLED(CONFIG_FIT) &&
> -		    !fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
> -			/*
> -			 * FIT images of type EFI_OS are started via command
> -			 * bootm. We should not use their boot device with the
> -			 * bootefi command.
> -			 */
> -			buffer = 0;
> -			buffer_size = 0;
> -		} else {
> -			log_debug("- not remembering image\n");
> -			return;
> -		}
> -	}
> -
> -	/* efi_set_bootdev() is typically called repeatedly, recover memory */
> -	efi_clear_bootdev();
> -
> -	image_addr = buffer;
> -	image_size = buffer_size;
> -
> -	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> -	if (ret == EFI_SUCCESS) {
> -		bootefi_device_path = device;
> -		if (image) {
> -			/* FIXME: image should not contain device */
> -			struct efi_device_path *image_tmp = image;
> -
> -			efi_dp_split_file_path(image, &device, &image);
> -			efi_free_pool(image_tmp);
> -		}
> -		bootefi_image_path = image;
> -		log_debug("- boot device %pD\n", device);
> -		if (image)
> -			log_debug("- image %pD\n", image);
> -	} else {
> -		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
> -		efi_clear_bootdev();
> -	}
> -}
> -
> -/**
> - * efi_env_set_load_options() - set load options from environment variable
> - *
> - * @handle:		the image handle
> - * @env_var:		name of the environment variable
> - * @load_options:	pointer to load options (output)
> - * Return:		status code
> - */
> -efi_status_t efi_env_set_load_options(efi_handle_t handle,
> -				      const char *env_var,
> -				      u16 **load_options)
> -{
> -	const char *env = env_get(env_var);
> -	size_t size;
> -	u16 *pos;
> -	efi_status_t ret;
> -
> -	*load_options = NULL;
> -	if (!env)
> -		return EFI_SUCCESS;
> -	size = sizeof(u16) * (utf8_utf16_strlen(env) + 1);
> -	pos = calloc(size, 1);
> -	if (!pos)
> -		return EFI_OUT_OF_RESOURCES;
> -	*load_options = pos;
> -	utf8_utf16_strcpy(&pos, env);
> -	ret = efi_set_load_options(handle, size, *load_options);
> -	if (ret != EFI_SUCCESS) {
> -		free(*load_options);
> -		*load_options = NULL;
> -	}
> -	return ret;
> -}
> -
> -#if !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
> -
> -/**
> - * copy_fdt() - Copy the device tree to a new location available to EFI
> - *
> - * The FDT is copied to a suitable location within the EFI memory map.
> - * Additional 12 KiB are added to the space in case the device tree needs to be
> - * expanded later with fdt_open_into().
> - *
> - * @fdtp:	On entry a pointer to the flattened device tree.
> - *		On exit a pointer to the copy of the flattened device tree.
> - *		FDT start
> - * Return:	status code
> - */
> -static efi_status_t copy_fdt(void **fdtp)
> -{
> -	unsigned long fdt_ram_start = -1L, fdt_pages;
> -	efi_status_t ret = 0;
> -	void *fdt, *new_fdt;
> -	u64 new_fdt_addr;
> -	uint fdt_size;
> -	int i;
> -
> -	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> -		u64 ram_start = gd->bd->bi_dram[i].start;
> -		u64 ram_size = gd->bd->bi_dram[i].size;
> -
> -		if (!ram_size)
> -			continue;
> -
> -		if (ram_start < fdt_ram_start)
> -			fdt_ram_start = ram_start;
> -	}
> -
> -	/*
> -	 * Give us at least 12 KiB of breathing room in case the device tree
> -	 * needs to be expanded later.
> -	 */
> -	fdt = *fdtp;
> -	fdt_pages = efi_size_in_pages(fdt_totalsize(fdt) + 0x3000);
> -	fdt_size = fdt_pages << EFI_PAGE_SHIFT;
> -
> -	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> -				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> -				 &new_fdt_addr);
> -	if (ret != EFI_SUCCESS) {
> -		log_err("ERROR: Failed to reserve space for FDT\n");
> -		goto done;
> -	}
> -	new_fdt = (void *)(uintptr_t)new_fdt_addr;
> -	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
> -	fdt_set_totalsize(new_fdt, fdt_size);
> -
> -	*fdtp = (void *)(uintptr_t)new_fdt_addr;
> -done:
> -	return ret;
> -}
> -
> -/**
> - * get_config_table() - get configuration table
> - *
> - * @guid:	GUID of the configuration table
> - * Return:	pointer to configuration table or NULL
> - */
> -static void *get_config_table(const efi_guid_t *guid)
> -{
> -	size_t i;
> -
> -	for (i = 0; i < systab.nr_tables; i++) {
> -		if (!guidcmp(guid, &systab.tables[i].guid))
> -			return systab.tables[i].table;
> -	}
> -	return NULL;
> -}
> -
> -#endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
> -
> -/**
> - * efi_install_fdt() - install device tree
> - *
> - * If fdt is not EFI_FDT_USE_INTERNAL, the device tree located at that memory
> - * address will be installed as configuration table, otherwise the device
> - * tree located at the address indicated by environment variable fdt_addr or as
> - * fallback fdtcontroladdr will be used.
> - *
> - * On architectures using ACPI tables device trees shall not be installed as
> - * configuration table.
> - *
> - * @fdt:	address of device tree or EFI_FDT_USE_INTERNAL to use
> - *		the hardware device tree as indicated by environment variable
> - *		fdt_addr or as fallback the internal device tree as indicated by
> - *		the environment variable fdtcontroladdr
> - * Return:	status code
> - */
> -efi_status_t efi_install_fdt(void *fdt)
> -{
> -	/*
> -	 * The EBBR spec requires that we have either an FDT or an ACPI table
> -	 * but not both.
> -	 */
> -#if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
> -	if (fdt) {
> -		log_warning("WARNING: Can't have ACPI table and device tree - ignoring DT.\n");
> -		return EFI_SUCCESS;
> -	}
> -#else
> -	struct bootm_headers img = { 0 };
> -	efi_status_t ret;
> -
> -	if (fdt == EFI_FDT_USE_INTERNAL) {
> -		const char *fdt_opt;
> -		uintptr_t fdt_addr;
> -
> -		/* Look for device tree that is already installed */
> -		if (get_config_table(&efi_guid_fdt))
> -			return EFI_SUCCESS;
> -		/* Check if there is a hardware device tree */
> -		fdt_opt = env_get("fdt_addr");
> -		/* Use our own device tree as fallback */
> -		if (!fdt_opt) {
> -			fdt_opt = env_get("fdtcontroladdr");
> -			if (!fdt_opt) {
> -				log_err("ERROR: need device tree\n");
> -				return EFI_NOT_FOUND;
> -			}
> -		}
> -		fdt_addr = hextoul(fdt_opt, NULL);
> -		if (!fdt_addr) {
> -			log_err("ERROR: invalid $fdt_addr or $fdtcontroladdr\n");
> -			return EFI_LOAD_ERROR;
> -		}
> -		fdt = map_sysmem(fdt_addr, 0);
> -	}
> -
> -	/* Install device tree */
> -	if (fdt_check_header(fdt)) {
> -		log_err("ERROR: invalid device tree\n");
> -		return EFI_LOAD_ERROR;
> -	}
> -
> -	/* Prepare device tree for payload */
> -	ret = copy_fdt(&fdt);
> -	if (ret) {
> -		log_err("ERROR: out of memory\n");
> -		return EFI_OUT_OF_RESOURCES;
> -	}
> -
> -	if (image_setup_libfdt(&img, fdt, NULL)) {
> -		log_err("ERROR: failed to process device tree\n");
> -		return EFI_LOAD_ERROR;
> -	}
> -
> -	/* Create memory reservations as indicated by the device tree */
> -	efi_carve_out_dt_rsv(fdt);
> -
> -	efi_try_purge_kaslr_seed(fdt);
> -
> -	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> -		ret = efi_tcg2_measure_dtb(fdt);
> -		if (ret == EFI_SECURITY_VIOLATION) {
> -			log_err("ERROR: failed to measure DTB\n");
> -			return ret;
> -		}
> -	}
> -
> -	/* Install device tree as UEFI table */
> -	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> -	if (ret != EFI_SUCCESS) {
> -		log_err("ERROR: failed to install device tree\n");
> -		return ret;
> -	}
> -#endif /* GENERATE_ACPI_TABLE */
> -
> -	return EFI_SUCCESS;
> -}
> -
> -/**
> - * do_bootefi_exec() - execute EFI binary
> - *
> - * The image indicated by @handle is started. When it returns the allocated
> - * memory for the @load_options is freed.
> - *
> - * @handle:		handle of loaded image
> - * @load_options:	load options
> - * Return:		status code
> - *
> - * Load the EFI binary into a newly assigned memory unwinding the relocation
> - * information, install the loaded image protocol, and call the binary.
> - */
> -static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
> -{
> -	efi_status_t ret;
> -	efi_uintn_t exit_data_size = 0;
> -	u16 *exit_data = NULL;
> -	struct efi_event *evt;
> -
> -	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
> -	switch_to_non_secure_mode();
> -
> -	/*
> -	 * The UEFI standard requires that the watchdog timer is set to five
> -	 * minutes when invoking an EFI boot option.
> -	 *
> -	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
> -	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
> -	 */
> -	ret = efi_set_watchdog(300);
> -	if (ret != EFI_SUCCESS) {
> -		log_err("ERROR: Failed to set watchdog timer\n");
> -		goto out;
> -	}
> -
> -	/* Call our payload! */
> -	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
> -	if (ret != EFI_SUCCESS) {
> -		log_err("## Application failed, r = %lu\n",
> -			ret & ~EFI_ERROR_MASK);
> -		if (exit_data) {
> -			log_err("## %ls\n", exit_data);
> -			efi_free_pool(exit_data);
> -		}
> -	}
> -
> -	efi_restore_gd();
> -
> -out:
> -	free(load_options);
> -
> -	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
> -		if (efi_initrd_deregister() != EFI_SUCCESS)
> -			log_err("Failed to remove loadfile2 for initrd\n");
> -	}
> -
> -	/* Notify EFI_EVENT_GROUP_RETURN_TO_EFIBOOTMGR event group. */
> -	list_for_each_entry(evt, &efi_events, link) {
> -		if (evt->group &&
> -		    !guidcmp(evt->group,
> -			     &efi_guid_event_group_return_to_efibootmgr)) {
> -			efi_signal_event(evt);
> -			EFI_CALL(systab.boottime->close_event(evt));
> -			break;
> -		}
> -	}
> -
> -	/* Control is returned to U-Boot, disable EFI watchdog */
> -	efi_set_watchdog(0);
> -
> -	return ret;
> -}
> -
>   /**
>    * efi_bootmgr_run() - execute EFI boot manager
>    * @fdt:	Flat device tree
> @@ -1548,100 +1152,3 @@ efi_status_t efi_bootmgr_run(void *fdt)
>
>   	return do_bootefi_exec(handle, load_options);
>   }
> -
> -/**
> - * efi_run_image() - run loaded UEFI image
> - *
> - * @source_buffer:	memory address of the UEFI image
> - * @source_size:	size of the UEFI image
> - * Return:		status code
> - */
> -efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> -{
> -	efi_handle_t mem_handle = NULL, handle;
> -	struct efi_device_path *file_path = NULL;
> -	struct efi_device_path *msg_path;
> -	efi_status_t ret, ret2;
> -	u16 *load_options;
> -
> -	if (!bootefi_device_path || !bootefi_image_path) {
> -		log_debug("Not loaded from disk\n");
> -		/*
> -		 * Special case for efi payload not loaded from disk,
> -		 * such as 'bootefi hello' or for example payload
> -		 * loaded directly into memory via JTAG, etc:
> -		 */
> -		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> -					    (uintptr_t)source_buffer,
> -					    source_size);
> -		/*
> -		 * Make sure that device for device_path exist
> -		 * in load_image(). Otherwise, shell and grub will fail.
> -		 */
> -		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
> -							       &efi_guid_device_path,
> -							       file_path, NULL);
> -		if (ret != EFI_SUCCESS)
> -			goto out;
> -		msg_path = file_path;
> -	} else {
> -		file_path = efi_dp_append(bootefi_device_path,
> -					  bootefi_image_path);
> -		msg_path = bootefi_image_path;
> -		log_debug("Loaded from disk\n");
> -	}
> -
> -	log_info("Booting %pD\n", msg_path);
> -
> -	ret = EFI_CALL(efi_load_image(false, efi_root, file_path, source_buffer,
> -				      source_size, &handle));
> -	if (ret != EFI_SUCCESS) {
> -		log_err("Loading image failed\n");
> -		goto out;
> -	}
> -
> -	/* Transfer environment variable as load options */
> -	ret = efi_env_set_load_options(handle, "bootargs", &load_options);
> -	if (ret != EFI_SUCCESS)
> -		goto out;
> -
> -	ret = do_bootefi_exec(handle, load_options);
> -
> -out:
> -	ret2 = efi_uninstall_multiple_protocol_interfaces(mem_handle,
> -							  &efi_guid_device_path,
> -							  file_path, NULL);
> -	efi_free_pool(file_path);
> -	return (ret != EFI_SUCCESS) ? ret : ret2;
> -}
> -
> -/**
> - * efi_binary_run() - run loaded UEFI image
> - *
> - * @image:	memory address of the UEFI image
> - * @size:	size of the UEFI image
> - * @fdt:	device-tree
> - *
> - * Execute an EFI binary image loaded at @image.
> - * @size may be zero if the binary is loaded with U-Boot load command.
> - *
> - * Return:	status code
> - */
> -efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> -{
> -	efi_status_t ret;
> -
> -	/* Initialize EFI drivers */
> -	ret = efi_init_obj_list();
> -	if (ret != EFI_SUCCESS) {
> -		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> -			ret & ~EFI_ERROR_MASK);
> -		return -1;
> -	}
> -
> -	ret = efi_install_fdt(fdt);
> -	if (ret != EFI_SUCCESS)
> -		return ret;
> -
> -	return efi_run_image(image, size);
> -}
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index ed7214f3a347..786d8a70e2ad 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -1090,7 +1090,8 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>   	if (path && !file)
>   		return EFI_INVALID_PARAMETER;
>
> -	if (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs"))  {
> +	if (IS_ENABLED(CONFIG_EFI_BINARY_EXEC) &&
> +	    (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs")))  {
>   		/* loadm command and semihosting */
>   		efi_get_image_parameters(&image_addr, &image_size);
>
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index cdfd16ea7742..79a2a579e901 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -1,17 +1,28 @@
>   // SPDX-License-Identifier: GPL-2.0+
>   /*
>    * Copyright (c) 2020, Linaro Limited
> + * For the code moved from cmd/bootefi.c
> + * Copyright (c) 2016 Alexander Graf
>    */
>
>   #define LOG_CATEGORY LOGC_EFI
> +#include <bootm.h>
>   #include <common.h>
> -#include <env.h>
> -#include <malloc.h>
>   #include <dm.h>
> -#include <fs.h>
>   #include <efi_load_initrd.h>
>   #include <efi_loader.h>
>   #include <efi_variable.h>
> +#include <env.h>
> +#include <fs.h>
> +#include <log.h>
> +#include <malloc.h>
> +#include <mapmem.h>
> +#include <vsprintf.h>
> +#include <asm/global_data.h>
> +#include <linux/libfdt.h>
> +#include <linux/libfdt_env.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
>
>   #if defined(CONFIG_CMD_EFIDEBUG) || defined(CONFIG_EFI_LOAD_FILE2_INITRD)
>   /* GUID used by Linux to identify the LoadFile2 protocol with the initrd */
> @@ -282,3 +293,485 @@ bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *inde
>
>   	return false;
>   }
> +
> +#if !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
> +
> +/**
> + * copy_fdt() - Copy the device tree to a new location available to EFI
> + *
> + * The FDT is copied to a suitable location within the EFI memory map.
> + * Additional 12 KiB are added to the space in case the device tree needs to be
> + * expanded later with fdt_open_into().
> + *
> + * @fdtp:	On entry a pointer to the flattened device tree.
> + *		On exit a pointer to the copy of the flattened device tree.
> + *		FDT start
> + * Return:	status code
> + */
> +static efi_status_t copy_fdt(void **fdtp)
> +{
> +	unsigned long fdt_ram_start = -1L, fdt_pages;
> +	efi_status_t ret = 0;
> +	void *fdt, *new_fdt;
> +	u64 new_fdt_addr;
> +	uint fdt_size;
> +	int i;
> +
> +	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> +		u64 ram_start = gd->bd->bi_dram[i].start;
> +		u64 ram_size = gd->bd->bi_dram[i].size;
> +
> +		if (!ram_size)
> +			continue;
> +
> +		if (ram_start < fdt_ram_start)
> +			fdt_ram_start = ram_start;
> +	}
> +
> +	/*
> +	 * Give us at least 12 KiB of breathing room in case the device tree
> +	 * needs to be expanded later.
> +	 */
> +	fdt = *fdtp;
> +	fdt_pages = efi_size_in_pages(fdt_totalsize(fdt) + 0x3000);
> +	fdt_size = fdt_pages << EFI_PAGE_SHIFT;
> +
> +	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> +				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> +				 &new_fdt_addr);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("ERROR: Failed to reserve space for FDT\n");
> +		goto done;
> +	}
> +	new_fdt = (void *)(uintptr_t)new_fdt_addr;
> +	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
> +	fdt_set_totalsize(new_fdt, fdt_size);
> +
> +	*fdtp = (void *)(uintptr_t)new_fdt_addr;
> +done:
> +	return ret;
> +}
> +
> +/**
> + * get_config_table() - get configuration table
> + *
> + * @guid:	GUID of the configuration table
> + * Return:	pointer to configuration table or NULL
> + */
> +static void *get_config_table(const efi_guid_t *guid)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < systab.nr_tables; i++) {
> +		if (!guidcmp(guid, &systab.tables[i].guid))
> +			return systab.tables[i].table;
> +	}
> +	return NULL;
> +}
> +
> +#endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
> +
> +/**
> + * efi_install_fdt() - install device tree
> + *
> + * If fdt is not EFI_FDT_USE_INTERNAL, the device tree located at that memory
> + * address will be installed as configuration table, otherwise the device
> + * tree located at the address indicated by environment variable fdt_addr or as
> + * fallback fdtcontroladdr will be used.
> + *
> + * On architectures using ACPI tables device trees shall not be installed as
> + * configuration table.
> + *
> + * @fdt:	address of device tree or EFI_FDT_USE_INTERNAL to use
> + *		the hardware device tree as indicated by environment variable
> + *		fdt_addr or as fallback the internal device tree as indicated by
> + *		the environment variable fdtcontroladdr
> + * Return:	status code
> + */
> +efi_status_t efi_install_fdt(void *fdt)
> +{
> +	/*
> +	 * The EBBR spec requires that we have either an FDT or an ACPI table
> +	 * but not both.
> +	 */
> +#if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
> +	if (fdt) {
> +		log_warning("WARNING: Can't have ACPI table and device tree - ignoring DT.\n");
> +		return EFI_SUCCESS;
> +	}
> +#else
> +	struct bootm_headers img = { 0 };
> +	efi_status_t ret;
> +
> +	if (fdt == EFI_FDT_USE_INTERNAL) {
> +		const char *fdt_opt;
> +		uintptr_t fdt_addr;
> +
> +		/* Look for device tree that is already installed */
> +		if (get_config_table(&efi_guid_fdt))
> +			return EFI_SUCCESS;
> +		/* Check if there is a hardware device tree */
> +		fdt_opt = env_get("fdt_addr");
> +		/* Use our own device tree as fallback */
> +		if (!fdt_opt) {
> +			fdt_opt = env_get("fdtcontroladdr");
> +			if (!fdt_opt) {
> +				log_err("ERROR: need device tree\n");
> +				return EFI_NOT_FOUND;
> +			}
> +		}
> +		fdt_addr = hextoul(fdt_opt, NULL);
> +		if (!fdt_addr) {
> +			log_err("ERROR: invalid $fdt_addr or $fdtcontroladdr\n");
> +			return EFI_LOAD_ERROR;
> +		}
> +		fdt = map_sysmem(fdt_addr, 0);
> +	}
> +
> +	/* Install device tree */
> +	if (fdt_check_header(fdt)) {
> +		log_err("ERROR: invalid device tree\n");
> +		return EFI_LOAD_ERROR;
> +	}
> +
> +	/* Prepare device tree for payload */
> +	ret = copy_fdt(&fdt);
> +	if (ret) {
> +		log_err("ERROR: out of memory\n");
> +		return EFI_OUT_OF_RESOURCES;
> +	}
> +
> +	if (image_setup_libfdt(&img, fdt, NULL)) {
> +		log_err("ERROR: failed to process device tree\n");
> +		return EFI_LOAD_ERROR;
> +	}
> +
> +	/* Create memory reservations as indicated by the device tree */
> +	efi_carve_out_dt_rsv(fdt);
> +
> +	efi_try_purge_kaslr_seed(fdt);
> +
> +	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> +		ret = efi_tcg2_measure_dtb(fdt);
> +		if (ret == EFI_SECURITY_VIOLATION) {
> +			log_err("ERROR: failed to measure DTB\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* Install device tree as UEFI table */
> +	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("ERROR: failed to install device tree\n");
> +		return ret;
> +	}
> +#endif /* GENERATE_ACPI_TABLE */
> +
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * do_bootefi_exec() - execute EFI binary
> + *
> + * The image indicated by @handle is started. When it returns the allocated
> + * memory for the @load_options is freed.
> + *
> + * @handle:		handle of loaded image
> + * @load_options:	load options
> + * Return:		status code
> + *
> + * Load the EFI binary into a newly assigned memory unwinding the relocation
> + * information, install the loaded image protocol, and call the binary.
> + */
> +efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
> +{
> +	efi_status_t ret;
> +	efi_uintn_t exit_data_size = 0;
> +	u16 *exit_data = NULL;
> +	struct efi_event *evt;
> +
> +	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
> +	switch_to_non_secure_mode();
> +
> +	/*
> +	 * The UEFI standard requires that the watchdog timer is set to five
> +	 * minutes when invoking an EFI boot option.
> +	 *
> +	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
> +	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
> +	 */
> +	ret = efi_set_watchdog(300);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("ERROR: Failed to set watchdog timer\n");
> +		goto out;
> +	}
> +
> +	/* Call our payload! */
> +	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
> +	if (ret != EFI_SUCCESS) {
> +		log_err("## Application failed, r = %lu\n",
> +			ret & ~EFI_ERROR_MASK);
> +		if (exit_data) {
> +			log_err("## %ls\n", exit_data);
> +			efi_free_pool(exit_data);
> +		}
> +	}
> +
> +	efi_restore_gd();
> +
> +out:
> +	free(load_options);
> +
> +	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
> +		if (efi_initrd_deregister() != EFI_SUCCESS)
> +			log_err("Failed to remove loadfile2 for initrd\n");
> +	}
> +
> +	/* Notify EFI_EVENT_GROUP_RETURN_TO_EFIBOOTMGR event group. */
> +	list_for_each_entry(evt, &efi_events, link) {
> +		if (evt->group &&
> +		    !guidcmp(evt->group,
> +			     &efi_guid_event_group_return_to_efibootmgr)) {
> +			efi_signal_event(evt);
> +			EFI_CALL(systab.boottime->close_event(evt));
> +			break;
> +		}
> +	}
> +
> +	/* Control is returned to U-Boot, disable EFI watchdog */
> +	efi_set_watchdog(0);
> +
> +	return ret;
> +}
> +
> +#if CONFIG_IS_ENABLED(EFI_BINARY_EXEC)
> +static struct efi_device_path *bootefi_image_path;
> +static struct efi_device_path *bootefi_device_path;
> +static void *image_addr;
> +static size_t image_size;
> +
> +/**
> + * efi_get_image_parameters() - return image parameters
> + *
> + * @img_addr:		address of loaded image in memory
> + * @img_size:		size of loaded image
> + */
> +void efi_get_image_parameters(void **img_addr, size_t *img_size)
> +{
> +	*img_addr = image_addr;
> +	*img_size = image_size;
> +}
> +
> +/**
> + * efi_clear_bootdev() - clear boot device
> + */
> +void efi_clear_bootdev(void)
> +{
> +	efi_free_pool(bootefi_device_path);
> +	efi_free_pool(bootefi_image_path);
> +	bootefi_device_path = NULL;
> +	bootefi_image_path = NULL;
> +	image_addr = NULL;
> +	image_size = 0;
> +}
> +
> +/**
> + * efi_set_bootdev() - set boot device
> + *
> + * This function is called when a file is loaded, e.g. via the 'load' command.
> + * We use the path to this file to inform the UEFI binary about the boot device.
> + *
> + * @dev:		device, e.g. "MMC"
> + * @devnr:		number of the device, e.g. "1:2"
> + * @path:		path to file loaded
> + * @buffer:		buffer with file loaded
> + * @buffer_size:	size of file loaded
> + */
> +void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
> +		     void *buffer, size_t buffer_size)
> +{
> +	struct efi_device_path *device, *image;
> +	efi_status_t ret;
> +
> +	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
> +		  devnr, path, buffer, buffer_size);
> +
> +	/* Forget overwritten image */
> +	if (buffer + buffer_size >= image_addr &&
> +	    image_addr + image_size >= buffer)
> +		efi_clear_bootdev();
> +
> +	/* Remember only PE-COFF and FIT images */
> +	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
> +		if (IS_ENABLED(CONFIG_FIT) &&
> +		    !fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
> +			/*
> +			 * FIT images of type EFI_OS are started via command
> +			 * bootm. We should not use their boot device with the
> +			 * bootefi command.
> +			 */
> +			buffer = 0;
> +			buffer_size = 0;
> +		} else {
> +			log_debug("- not remembering image\n");
> +			return;
> +		}
> +	}
> +
> +	/* efi_set_bootdev() is typically called repeatedly, recover memory */
> +	efi_clear_bootdev();
> +
> +	image_addr = buffer;
> +	image_size = buffer_size;
> +
> +	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> +	if (ret == EFI_SUCCESS) {
> +		bootefi_device_path = device;
> +		if (image) {
> +			/* FIXME: image should not contain device */
> +			struct efi_device_path *image_tmp = image;
> +
> +			efi_dp_split_file_path(image, &device, &image);
> +			efi_free_pool(image_tmp);
> +		}
> +		bootefi_image_path = image;
> +		log_debug("- boot device %pD\n", device);
> +		if (image)
> +			log_debug("- image %pD\n", image);
> +	} else {
> +		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
> +		efi_clear_bootdev();
> +	}
> +}
> +
> +/**
> + * efi_env_set_load_options() - set load options from environment variable
> + *
> + * @handle:		the image handle
> + * @env_var:		name of the environment variable
> + * @load_options:	pointer to load options (output)
> + * Return:		status code
> + */
> +efi_status_t efi_env_set_load_options(efi_handle_t handle,
> +				      const char *env_var,
> +				      u16 **load_options)
> +{
> +	const char *env = env_get(env_var);
> +	size_t size;
> +	u16 *pos;
> +	efi_status_t ret;
> +
> +	*load_options = NULL;
> +	if (!env)
> +		return EFI_SUCCESS;
> +	size = sizeof(u16) * (utf8_utf16_strlen(env) + 1);
> +	pos = calloc(size, 1);
> +	if (!pos)
> +		return EFI_OUT_OF_RESOURCES;
> +	*load_options = pos;
> +	utf8_utf16_strcpy(&pos, env);
> +	ret = efi_set_load_options(handle, size, *load_options);
> +	if (ret != EFI_SUCCESS) {
> +		free(*load_options);
> +		*load_options = NULL;
> +	}
> +	return ret;
> +}
> +
> +/**
> + * efi_run_image() - run loaded UEFI image
> + *
> + * @source_buffer:	memory address of the UEFI image
> + * @source_size:	size of the UEFI image
> + * Return:		status code
> + */
> +efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> +{
> +	efi_handle_t mem_handle = NULL, handle;
> +	struct efi_device_path *file_path = NULL;
> +	struct efi_device_path *msg_path;
> +	efi_status_t ret, ret2;
> +	u16 *load_options;
> +
> +	if (!bootefi_device_path || !bootefi_image_path) {
> +		log_debug("Not loaded from disk\n");
> +		/*
> +		 * Special case for efi payload not loaded from disk,
> +		 * such as 'bootefi hello' or for example payload
> +		 * loaded directly into memory via JTAG, etc:
> +		 */
> +		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					    (uintptr_t)source_buffer,
> +					    source_size);
> +		/*
> +		 * Make sure that device for device_path exist
> +		 * in load_image(). Otherwise, shell and grub will fail.
> +		 */
> +		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
> +							       &efi_guid_device_path,
> +							       file_path, NULL);
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +		msg_path = file_path;
> +	} else {
> +		file_path = efi_dp_append(bootefi_device_path,
> +					  bootefi_image_path);
> +		msg_path = bootefi_image_path;
> +		log_debug("Loaded from disk\n");
> +	}
> +
> +	log_info("Booting %pD\n", msg_path);
> +
> +	ret = EFI_CALL(efi_load_image(false, efi_root, file_path, source_buffer,
> +				      source_size, &handle));
> +	if (ret != EFI_SUCCESS) {
> +		log_err("Loading image failed\n");
> +		goto out;
> +	}
> +
> +	/* Transfer environment variable as load options */
> +	ret = efi_env_set_load_options(handle, "bootargs", &load_options);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = do_bootefi_exec(handle, load_options);
> +
> +out:
> +	ret2 = efi_uninstall_multiple_protocol_interfaces(mem_handle,
> +							  &efi_guid_device_path,
> +							  file_path, NULL);
> +	efi_free_pool(file_path);
> +	return (ret != EFI_SUCCESS) ? ret : ret2;
> +}
> +
> +/**
> + * efi_binary_run() - run loaded UEFI image
> + *
> + * @image:	memory address of the UEFI image
> + * @size:	size of the UEFI image
> + * @fdt:	device-tree
> + *
> + * Execute an EFI binary image loaded at @image.
> + * @size may be zero if the binary is loaded with U-Boot load command.
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> +{
> +	efi_status_t ret;
> +
> +	/* Initialize EFI drivers */
> +	ret = efi_init_obj_list();
> +	if (ret != EFI_SUCCESS) {
> +		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> +			ret & ~EFI_ERROR_MASK);
> +		return -1;
> +	}
> +
> +	ret = efi_install_fdt(fdt);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	return efi_run_image(image, size);
> +}
> +#endif /* CONFIG_BINARY_EXEC */

Re: [PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c

[AKASHI Takahiro]

On Mon, Dec 25, 2023 at 10:17:06AM +0100, Heinrich Schuchardt wrote:
> On 12/18/23 03:38, AKASHI Takahiro wrote:
> > Some code moved from cmd/bootefi.c is actually necessary only for "bootefi
> > <addr>" command (starting an image manually loaded by a user using U-Boot
> > load commands or other methods (like JTAG debugger).
> > 
> > The code will never been opted out as unused code by a compiler which
> > doesn't know how EFI boot manager is implemented. So introduce a new
> > configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out
> > explicitly.
> 
> We build with -ffunction-sections. The linker removes unreferenced
> functions.

Yes, I know but I also think it would be better in terms of readability
and maintainability to add a new config option and separate EFI_BINARY_EXEC
portion from BOOTEFI_BOOTMGR as these two functions share almost nothing
(except efi_install_fdt()).

> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   boot/Kconfig                     |   4 +-
> >   cmd/Kconfig                      |   6 +-
> >   include/efi_loader.h             |  28 +-
> >   lib/efi_loader/Kconfig           |   9 +
> >   lib/efi_loader/efi_bootmgr.c     | 493 ------------------------------
> >   lib/efi_loader/efi_device_path.c |   3 +-
> >   lib/efi_loader/efi_helper.c      | 499 ++++++++++++++++++++++++++++++-
> 
> We expect that after each patch we can compile the code. This requires
> that the Makefile change is in the same patch as the creation of
> efi_helper.c.

Please remember that efi_helper.c is not a new file.
If you like, as Simon suggested, I will move "499" lines of code
into a new file, efi_boot.c and then add:
   obj-$(CONFIG_EFI_BINARY_EXEC) := efi_boot.o

> >   7 files changed, 529 insertions(+), 513 deletions(-)
> > 
> > diff --git a/boot/Kconfig b/boot/Kconfig
> > index 987ca7314117..8ab7e6f63d34 100644
> > --- a/boot/Kconfig
> > +++ b/boot/Kconfig
> > @@ -523,7 +523,7 @@ config BOOTMETH_EXTLINUX_PXE
> > 
> >   config BOOTMETH_EFILOADER
> >   	bool "Bootdev support for EFI boot"
> > -	depends on BOOTEFI_BOOTMGR
> > +	depends on EFI_BINARY_EXEC
> 
> Why do we need a symbol CONFIG_EFI_BINARY_EXEC? CONFIG_EFI_LOADER=y
> without the ability to execute an EFI binary makes no sense to me.

It's up to users. It allows them to configure U-Boot with EFI_LOADER and
EFI_BOOTMGR only.

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >   	default y
> >   	help
> >   	  Enables support for EFI boot using bootdevs. This makes the
> > @@ -558,7 +558,7 @@ config BOOTMETH_DISTRO
> >   	select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
> >   	select BOOTMETH_EXTLINUX  # E.g. Debian uses these
> >   	select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
> > -	select BOOTMETH_EFILOADER if BOOTEFI_BOOTMGR # E.g. Ubuntu uses this
> > +	select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
> > 
> >   config SPL_BOOTMETH_VBE
> >   	bool "Bootdev support for Verified Boot for Embedded (SPL)"
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 24bfbe505722..2c993496b70e 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -273,7 +273,7 @@ config CMD_BOOTMETH
> > 
> >   config BOOTM_EFI
> >   	bool "Support booting UEFI FIT images"
> > -	depends on BOOTEFI_BOOTMGR && CMD_BOOTM && FIT
> > +	depends on EFI_BINARY_EXEC && CMD_BOOTM && FIT
> >   	default y
> >   	help
> >   	  Support booting UEFI FIT images via the bootm command.
> > @@ -365,7 +365,7 @@ config CMD_BOOTEFI
> >   if CMD_BOOTEFI
> >   config CMD_BOOTEFI_BINARY
> >   	bool "Allow booting an EFI binary directly"
> > -	depends on BOOTEFI_BOOTMGR
> > +	depends on EFI_BINARY_EXEC
> >   	default y
> >   	help
> >   	  Select this option to enable direct execution of binary at 'bootefi'.
> > @@ -395,7 +395,7 @@ config CMD_BOOTEFI_HELLO_COMPILE
> > 
> >   config CMD_BOOTEFI_HELLO
> >   	bool "Allow booting a standard EFI hello world for testing"
> > -	depends on CMD_BOOTEFI_HELLO_COMPILE
> > +	depends on CMD_BOOTEFI_BINARY && CMD_BOOTEFI_HELLO_COMPILE
> >   	default y if CMD_BOOTEFI_SELFTEST
> >   	help
> >   	  This adds a standard EFI hello world application to U-Boot so that
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 34e7fbbf1840..484c9fad239f 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -90,11 +90,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
> >    * back to u-boot world
> >    */
> >   void efi_restore_gd(void);
> > -/* Call this to unset the current device name */
> > -void efi_clear_bootdev(void);
> > -/* Call this to set the current device name */
> > -void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
> > -		     void *buffer, size_t buffer_size);
> > +
> >   /* Called by networking code to memorize the dhcp ack package */
> >   void efi_net_set_dhcp_ack(void *pkt, int len);
> >   /* Print information about all loaded images */
> > @@ -116,10 +112,6 @@ static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
> > 
> >   /* No loader configured, stub out EFI_ENTRY */
> >   static inline void efi_restore_gd(void) { }
> > -static inline void efi_clear_bootdev(void) { }
> > -static inline void efi_set_bootdev(const char *dev, const char *devnr,
> > -				   const char *path, void *buffer,
> > -				   size_t buffer_size) { }
> >   static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
> >   static inline void efi_print_image_infos(void *pc) { }
> >   static inline efi_status_t efi_launch_capsules(void)
> > @@ -129,6 +121,20 @@ static inline efi_status_t efi_launch_capsules(void)
> > 
> >   #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
> > 
> > +#if CONFIG_IS_ENABLED(EFI_BINARY_EXEC)
> > +/* Call this to unset the current device name */
> > +void efi_clear_bootdev(void);
> > +/* Call this to set the current device name */
> > +void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
> > +		     void *buffer, size_t buffer_size);
> > +#else
> > +static inline void efi_clear_bootdev(void) { }
> > +
> > +static inline void efi_set_bootdev(const char *dev, const char *devnr,
> > +				   const char *path, void *buffer,
> > +				   size_t buffer_size) { }
> > +#endif
> > +
> >   /* Maximum number of configuration tables */
> >   #define EFI_MAX_CONFIGURATION_TABLES 16
> > 
> > @@ -541,8 +547,8 @@ efi_status_t efi_env_set_load_options(efi_handle_t handle, const char *env_var,
> >   				      u16 **load_options);
> >   /* Install device tree */
> >   efi_status_t efi_install_fdt(void *fdt);
> > -/* Run loaded UEFI image */
> > -efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size);
> > +/* Execute loaded UEFI image */
> > +efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options);
> >   /* Run loaded UEFI image with given fdt */
> >   efi_status_t efi_binary_run(void *image, size_t size, void *fdt);
> >   /* Initialize variable services */
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index ea807342f02f..64f2f1cdd161 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -32,6 +32,15 @@ config EFI_LOADER
> > 
> >   if EFI_LOADER
> > 
> > +config EFI_BINARY_EXEC
> > +	bool "Execute UEFI binary"
> > +	default y
> > +	help
> > +	  Select this option if you want to execute the UEFI binary after
> > +	  loading it with U-Boot load commands or other methods.
> > +	  You may enable CMD_BOOTEFI_BINARY so that you can use bootefi
> > +	  command to do that.
> > +
> >   config BOOTEFI_BOOTMGR
> >   	bool "UEFI Boot Manager"
> >   	default y
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 56d97f23827b..e3b27cd7db3e 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -3,8 +3,6 @@
> >    *  EFI boot manager
> >    *
> >    *  Copyright (c) 2017 Rob Clark
> > - *  For the code moved from cmd/bootefi.c
> > - *  Copyright (c) 2016 Alexander Graf
> >    */
> > 
> >   #define LOG_CATEGORY LOGC_EFI
> > @@ -22,17 +20,6 @@
> >   #include <efi_variable.h>
> >   #include <asm/unaligned.h>
> > 
> > -/* TODO: temporarily added here; clean up later */
> > -#include <bootm.h>
> > -#include <efi_selftest.h>
> > -#include <env.h>
> > -#include <mapmem.h>
> > -#include <asm/global_data.h>
> > -#include <linux/libfdt.h>
> > -#include <linux/libfdt_env.h>
> > -
> > -DECLARE_GLOBAL_DATA_PTR;
> > -
> >   static const struct efi_boot_services *bs;
> >   static const struct efi_runtime_services *rs;
> > 
> > @@ -1129,389 +1116,6 @@ out:
> >   	return ret;
> >   }
> > 
> > -static struct efi_device_path *bootefi_image_path;
> > -static struct efi_device_path *bootefi_device_path;
> > -static void *image_addr;
> > -static size_t image_size;
> > -
> > -/**
> > - * efi_get_image_parameters() - return image parameters
> > - *
> > - * @img_addr:		address of loaded image in memory
> > - * @img_size:		size of loaded image
> > - */
> > -void efi_get_image_parameters(void **img_addr, size_t *img_size)
> > -{
> > -	*img_addr = image_addr;
> > -	*img_size = image_size;
> > -}
> > -
> > -/**
> > - * efi_clear_bootdev() - clear boot device
> > - */
> > -void efi_clear_bootdev(void)
> > -{
> > -	efi_free_pool(bootefi_device_path);
> > -	efi_free_pool(bootefi_image_path);
> > -	bootefi_device_path = NULL;
> > -	bootefi_image_path = NULL;
> > -	image_addr = NULL;
> > -	image_size = 0;
> > -}
> > -
> > -/**
> > - * efi_set_bootdev() - set boot device
> > - *
> > - * This function is called when a file is loaded, e.g. via the 'load' command.
> > - * We use the path to this file to inform the UEFI binary about the boot device.
> > - *
> > - * @dev:		device, e.g. "MMC"
> > - * @devnr:		number of the device, e.g. "1:2"
> > - * @path:		path to file loaded
> > - * @buffer:		buffer with file loaded
> > - * @buffer_size:	size of file loaded
> > - */
> > -void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
> > -		     void *buffer, size_t buffer_size)
> > -{
> > -	struct efi_device_path *device, *image;
> > -	efi_status_t ret;
> > -
> > -	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
> > -		  devnr, path, buffer, buffer_size);
> > -
> > -	/* Forget overwritten image */
> > -	if (buffer + buffer_size >= image_addr &&
> > -	    image_addr + image_size >= buffer)
> > -		efi_clear_bootdev();
> > -
> > -	/* Remember only PE-COFF and FIT images */
> > -	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
> > -		if (IS_ENABLED(CONFIG_FIT) &&
> > -		    !fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
> > -			/*
> > -			 * FIT images of type EFI_OS are started via command
> > -			 * bootm. We should not use their boot device with the
> > -			 * bootefi command.
> > -			 */
> > -			buffer = 0;
> > -			buffer_size = 0;
> > -		} else {
> > -			log_debug("- not remembering image\n");
> > -			return;
> > -		}
> > -	}
> > -
> > -	/* efi_set_bootdev() is typically called repeatedly, recover memory */
> > -	efi_clear_bootdev();
> > -
> > -	image_addr = buffer;
> > -	image_size = buffer_size;
> > -
> > -	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> > -	if (ret == EFI_SUCCESS) {
> > -		bootefi_device_path = device;
> > -		if (image) {
> > -			/* FIXME: image should not contain device */
> > -			struct efi_device_path *image_tmp = image;
> > -
> > -			efi_dp_split_file_path(image, &device, &image);
> > -			efi_free_pool(image_tmp);
> > -		}
> > -		bootefi_image_path = image;
> > -		log_debug("- boot device %pD\n", device);
> > -		if (image)
> > -			log_debug("- image %pD\n", image);
> > -	} else {
> > -		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
> > -		efi_clear_bootdev();
> > -	}
> > -}
> > -
> > -/**
> > - * efi_env_set_load_options() - set load options from environment variable
> > - *
> > - * @handle:		the image handle
> > - * @env_var:		name of the environment variable
> > - * @load_options:	pointer to load options (output)
> > - * Return:		status code
> > - */
> > -efi_status_t efi_env_set_load_options(efi_handle_t handle,
> > -				      const char *env_var,
> > -				      u16 **load_options)
> > -{
> > -	const char *env = env_get(env_var);
> > -	size_t size;
> > -	u16 *pos;
> > -	efi_status_t ret;
> > -
> > -	*load_options = NULL;
> > -	if (!env)
> > -		return EFI_SUCCESS;
> > -	size = sizeof(u16) * (utf8_utf16_strlen(env) + 1);
> > -	pos = calloc(size, 1);
> > -	if (!pos)
> > -		return EFI_OUT_OF_RESOURCES;
> > -	*load_options = pos;
> > -	utf8_utf16_strcpy(&pos, env);
> > -	ret = efi_set_load_options(handle, size, *load_options);
> > -	if (ret != EFI_SUCCESS) {
> > -		free(*load_options);
> > -		*load_options = NULL;
> > -	}
> > -	return ret;
> > -}
> > -
> > -#if !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
> > -
> > -/**
> > - * copy_fdt() - Copy the device tree to a new location available to EFI
> > - *
> > - * The FDT is copied to a suitable location within the EFI memory map.
> > - * Additional 12 KiB are added to the space in case the device tree needs to be
> > - * expanded later with fdt_open_into().
> > - *
> > - * @fdtp:	On entry a pointer to the flattened device tree.
> > - *		On exit a pointer to the copy of the flattened device tree.
> > - *		FDT start
> > - * Return:	status code
> > - */
> > -static efi_status_t copy_fdt(void **fdtp)
> > -{
> > -	unsigned long fdt_ram_start = -1L, fdt_pages;
> > -	efi_status_t ret = 0;
> > -	void *fdt, *new_fdt;
> > -	u64 new_fdt_addr;
> > -	uint fdt_size;
> > -	int i;
> > -
> > -	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> > -		u64 ram_start = gd->bd->bi_dram[i].start;
> > -		u64 ram_size = gd->bd->bi_dram[i].size;
> > -
> > -		if (!ram_size)
> > -			continue;
> > -
> > -		if (ram_start < fdt_ram_start)
> > -			fdt_ram_start = ram_start;
> > -	}
> > -
> > -	/*
> > -	 * Give us at least 12 KiB of breathing room in case the device tree
> > -	 * needs to be expanded later.
> > -	 */
> > -	fdt = *fdtp;
> > -	fdt_pages = efi_size_in_pages(fdt_totalsize(fdt) + 0x3000);
> > -	fdt_size = fdt_pages << EFI_PAGE_SHIFT;
> > -
> > -	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> > -				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> > -				 &new_fdt_addr);
> > -	if (ret != EFI_SUCCESS) {
> > -		log_err("ERROR: Failed to reserve space for FDT\n");
> > -		goto done;
> > -	}
> > -	new_fdt = (void *)(uintptr_t)new_fdt_addr;
> > -	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
> > -	fdt_set_totalsize(new_fdt, fdt_size);
> > -
> > -	*fdtp = (void *)(uintptr_t)new_fdt_addr;
> > -done:
> > -	return ret;
> > -}
> > -
> > -/**
> > - * get_config_table() - get configuration table
> > - *
> > - * @guid:	GUID of the configuration table
> > - * Return:	pointer to configuration table or NULL
> > - */
> > -static void *get_config_table(const efi_guid_t *guid)
> > -{
> > -	size_t i;
> > -
> > -	for (i = 0; i < systab.nr_tables; i++) {
> > -		if (!guidcmp(guid, &systab.tables[i].guid))
> > -			return systab.tables[i].table;
> > -	}
> > -	return NULL;
> > -}
> > -
> > -#endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
> > -
> > -/**
> > - * efi_install_fdt() - install device tree
> > - *
> > - * If fdt is not EFI_FDT_USE_INTERNAL, the device tree located at that memory
> > - * address will be installed as configuration table, otherwise the device
> > - * tree located at the address indicated by environment variable fdt_addr or as
> > - * fallback fdtcontroladdr will be used.
> > - *
> > - * On architectures using ACPI tables device trees shall not be installed as
> > - * configuration table.
> > - *
> > - * @fdt:	address of device tree or EFI_FDT_USE_INTERNAL to use
> > - *		the hardware device tree as indicated by environment variable
> > - *		fdt_addr or as fallback the internal device tree as indicated by
> > - *		the environment variable fdtcontroladdr
> > - * Return:	status code
> > - */
> > -efi_status_t efi_install_fdt(void *fdt)
> > -{
> > -	/*
> > -	 * The EBBR spec requires that we have either an FDT or an ACPI table
> > -	 * but not both.
> > -	 */
> > -#if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
> > -	if (fdt) {
> > -		log_warning("WARNING: Can't have ACPI table and device tree - ignoring DT.\n");
> > -		return EFI_SUCCESS;
> > -	}
> > -#else
> > -	struct bootm_headers img = { 0 };
> > -	efi_status_t ret;
> > -
> > -	if (fdt == EFI_FDT_USE_INTERNAL) {
> > -		const char *fdt_opt;
> > -		uintptr_t fdt_addr;
> > -
> > -		/* Look for device tree that is already installed */
> > -		if (get_config_table(&efi_guid_fdt))
> > -			return EFI_SUCCESS;
> > -		/* Check if there is a hardware device tree */
> > -		fdt_opt = env_get("fdt_addr");
> > -		/* Use our own device tree as fallback */
> > -		if (!fdt_opt) {
> > -			fdt_opt = env_get("fdtcontroladdr");
> > -			if (!fdt_opt) {
> > -				log_err("ERROR: need device tree\n");
> > -				return EFI_NOT_FOUND;
> > -			}
> > -		}
> > -		fdt_addr = hextoul(fdt_opt, NULL);
> > -		if (!fdt_addr) {
> > -			log_err("ERROR: invalid $fdt_addr or $fdtcontroladdr\n");
> > -			return EFI_LOAD_ERROR;
> > -		}
> > -		fdt = map_sysmem(fdt_addr, 0);
> > -	}
> > -
> > -	/* Install device tree */
> > -	if (fdt_check_header(fdt)) {
> > -		log_err("ERROR: invalid device tree\n");
> > -		return EFI_LOAD_ERROR;
> > -	}
> > -
> > -	/* Prepare device tree for payload */
> > -	ret = copy_fdt(&fdt);
> > -	if (ret) {
> > -		log_err("ERROR: out of memory\n");
> > -		return EFI_OUT_OF_RESOURCES;
> > -	}
> > -
> > -	if (image_setup_libfdt(&img, fdt, NULL)) {
> > -		log_err("ERROR: failed to process device tree\n");
> > -		return EFI_LOAD_ERROR;
> > -	}
> > -
> > -	/* Create memory reservations as indicated by the device tree */
> > -	efi_carve_out_dt_rsv(fdt);
> > -
> > -	efi_try_purge_kaslr_seed(fdt);
> > -
> > -	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> > -		ret = efi_tcg2_measure_dtb(fdt);
> > -		if (ret == EFI_SECURITY_VIOLATION) {
> > -			log_err("ERROR: failed to measure DTB\n");
> > -			return ret;
> > -		}
> > -	}
> > -
> > -	/* Install device tree as UEFI table */
> > -	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> > -	if (ret != EFI_SUCCESS) {
> > -		log_err("ERROR: failed to install device tree\n");
> > -		return ret;
> > -	}
> > -#endif /* GENERATE_ACPI_TABLE */
> > -
> > -	return EFI_SUCCESS;
> > -}
> > -
> > -/**
> > - * do_bootefi_exec() - execute EFI binary
> > - *
> > - * The image indicated by @handle is started. When it returns the allocated
> > - * memory for the @load_options is freed.
> > - *
> > - * @handle:		handle of loaded image
> > - * @load_options:	load options
> > - * Return:		status code
> > - *
> > - * Load the EFI binary into a newly assigned memory unwinding the relocation
> > - * information, install the loaded image protocol, and call the binary.
> > - */
> > -static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
> > -{
> > -	efi_status_t ret;
> > -	efi_uintn_t exit_data_size = 0;
> > -	u16 *exit_data = NULL;
> > -	struct efi_event *evt;
> > -
> > -	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
> > -	switch_to_non_secure_mode();
> > -
> > -	/*
> > -	 * The UEFI standard requires that the watchdog timer is set to five
> > -	 * minutes when invoking an EFI boot option.
> > -	 *
> > -	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
> > -	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
> > -	 */
> > -	ret = efi_set_watchdog(300);
> > -	if (ret != EFI_SUCCESS) {
> > -		log_err("ERROR: Failed to set watchdog timer\n");
> > -		goto out;
> > -	}
> > -
> > -	/* Call our payload! */
> > -	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
> > -	if (ret != EFI_SUCCESS) {
> > -		log_err("## Application failed, r = %lu\n",
> > -			ret & ~EFI_ERROR_MASK);
> > -		if (exit_data) {
> > -			log_err("## %ls\n", exit_data);
> > -			efi_free_pool(exit_data);
> > -		}
> > -	}
> > -
> > -	efi_restore_gd();
> > -
> > -out:
> > -	free(load_options);
> > -
> > -	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
> > -		if (efi_initrd_deregister() != EFI_SUCCESS)
> > -			log_err("Failed to remove loadfile2 for initrd\n");
> > -	}
> > -
> > -	/* Notify EFI_EVENT_GROUP_RETURN_TO_EFIBOOTMGR event group. */
> > -	list_for_each_entry(evt, &efi_events, link) {
> > -		if (evt->group &&
> > -		    !guidcmp(evt->group,
> > -			     &efi_guid_event_group_return_to_efibootmgr)) {
> > -			efi_signal_event(evt);
> > -			EFI_CALL(systab.boottime->close_event(evt));
> > -			break;
> > -		}
> > -	}
> > -
> > -	/* Control is returned to U-Boot, disable EFI watchdog */
> > -	efi_set_watchdog(0);
> > -
> > -	return ret;
> > -}
> > -
> >   /**
> >    * efi_bootmgr_run() - execute EFI boot manager
> >    * @fdt:	Flat device tree
> > @@ -1548,100 +1152,3 @@ efi_status_t efi_bootmgr_run(void *fdt)
> > 
> >   	return do_bootefi_exec(handle, load_options);
> >   }
> > -
> > -/**
> > - * efi_run_image() - run loaded UEFI image
> > - *
> > - * @source_buffer:	memory address of the UEFI image
> > - * @source_size:	size of the UEFI image
> > - * Return:		status code
> > - */
> > -efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> > -{
> > -	efi_handle_t mem_handle = NULL, handle;
> > -	struct efi_device_path *file_path = NULL;
> > -	struct efi_device_path *msg_path;
> > -	efi_status_t ret, ret2;
> > -	u16 *load_options;
> > -
> > -	if (!bootefi_device_path || !bootefi_image_path) {
> > -		log_debug("Not loaded from disk\n");
> > -		/*
> > -		 * Special case for efi payload not loaded from disk,
> > -		 * such as 'bootefi hello' or for example payload
> > -		 * loaded directly into memory via JTAG, etc:
> > -		 */
> > -		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> > -					    (uintptr_t)source_buffer,
> > -					    source_size);
> > -		/*
> > -		 * Make sure that device for device_path exist
> > -		 * in load_image(). Otherwise, shell and grub will fail.
> > -		 */
> > -		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
> > -							       &efi_guid_device_path,
> > -							       file_path, NULL);
> > -		if (ret != EFI_SUCCESS)
> > -			goto out;
> > -		msg_path = file_path;
> > -	} else {
> > -		file_path = efi_dp_append(bootefi_device_path,
> > -					  bootefi_image_path);
> > -		msg_path = bootefi_image_path;
> > -		log_debug("Loaded from disk\n");
> > -	}
> > -
> > -	log_info("Booting %pD\n", msg_path);
> > -
> > -	ret = EFI_CALL(efi_load_image(false, efi_root, file_path, source_buffer,
> > -				      source_size, &handle));
> > -	if (ret != EFI_SUCCESS) {
> > -		log_err("Loading image failed\n");
> > -		goto out;
> > -	}
> > -
> > -	/* Transfer environment variable as load options */
> > -	ret = efi_env_set_load_options(handle, "bootargs", &load_options);
> > -	if (ret != EFI_SUCCESS)
> > -		goto out;
> > -
> > -	ret = do_bootefi_exec(handle, load_options);
> > -
> > -out:
> > -	ret2 = efi_uninstall_multiple_protocol_interfaces(mem_handle,
> > -							  &efi_guid_device_path,
> > -							  file_path, NULL);
> > -	efi_free_pool(file_path);
> > -	return (ret != EFI_SUCCESS) ? ret : ret2;
> > -}
> > -
> > -/**
> > - * efi_binary_run() - run loaded UEFI image
> > - *
> > - * @image:	memory address of the UEFI image
> > - * @size:	size of the UEFI image
> > - * @fdt:	device-tree
> > - *
> > - * Execute an EFI binary image loaded at @image.
> > - * @size may be zero if the binary is loaded with U-Boot load command.
> > - *
> > - * Return:	status code
> > - */
> > -efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> > -{
> > -	efi_status_t ret;
> > -
> > -	/* Initialize EFI drivers */
> > -	ret = efi_init_obj_list();
> > -	if (ret != EFI_SUCCESS) {
> > -		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > -			ret & ~EFI_ERROR_MASK);
> > -		return -1;
> > -	}
> > -
> > -	ret = efi_install_fdt(fdt);
> > -	if (ret != EFI_SUCCESS)
> > -		return ret;
> > -
> > -	return efi_run_image(image, size);
> > -}
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index ed7214f3a347..786d8a70e2ad 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -1090,7 +1090,8 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
> >   	if (path && !file)
> >   		return EFI_INVALID_PARAMETER;
> > 
> > -	if (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs"))  {
> > +	if (IS_ENABLED(CONFIG_EFI_BINARY_EXEC) &&
> > +	    (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs")))  {
> >   		/* loadm command and semihosting */
> >   		efi_get_image_parameters(&image_addr, &image_size);
> > 
> > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > index cdfd16ea7742..79a2a579e901 100644
> > --- a/lib/efi_loader/efi_helper.c
> > +++ b/lib/efi_loader/efi_helper.c
> > @@ -1,17 +1,28 @@
> >   // SPDX-License-Identifier: GPL-2.0+
> >   /*
> >    * Copyright (c) 2020, Linaro Limited
> > + * For the code moved from cmd/bootefi.c
> > + * Copyright (c) 2016 Alexander Graf
> >    */
> > 
> >   #define LOG_CATEGORY LOGC_EFI
> > +#include <bootm.h>
> >   #include <common.h>
> > -#include <env.h>
> > -#include <malloc.h>
> >   #include <dm.h>
> > -#include <fs.h>
> >   #include <efi_load_initrd.h>
> >   #include <efi_loader.h>
> >   #include <efi_variable.h>
> > +#include <env.h>
> > +#include <fs.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +#include <mapmem.h>
> > +#include <vsprintf.h>
> > +#include <asm/global_data.h>
> > +#include <linux/libfdt.h>
> > +#include <linux/libfdt_env.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > 
> >   #if defined(CONFIG_CMD_EFIDEBUG) || defined(CONFIG_EFI_LOAD_FILE2_INITRD)
> >   /* GUID used by Linux to identify the LoadFile2 protocol with the initrd */
> > @@ -282,3 +293,485 @@ bool efi_search_bootorder(u16 *bootorder, efi_uintn_t num, u32 target, u32 *inde
> > 
> >   	return false;
> >   }
> > +
> > +#if !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
> > +
> > +/**
> > + * copy_fdt() - Copy the device tree to a new location available to EFI
> > + *
> > + * The FDT is copied to a suitable location within the EFI memory map.
> > + * Additional 12 KiB are added to the space in case the device tree needs to be
> > + * expanded later with fdt_open_into().
> > + *
> > + * @fdtp:	On entry a pointer to the flattened device tree.
> > + *		On exit a pointer to the copy of the flattened device tree.
> > + *		FDT start
> > + * Return:	status code
> > + */
> > +static efi_status_t copy_fdt(void **fdtp)
> > +{
> > +	unsigned long fdt_ram_start = -1L, fdt_pages;
> > +	efi_status_t ret = 0;
> > +	void *fdt, *new_fdt;
> > +	u64 new_fdt_addr;
> > +	uint fdt_size;
> > +	int i;
> > +
> > +	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> > +		u64 ram_start = gd->bd->bi_dram[i].start;
> > +		u64 ram_size = gd->bd->bi_dram[i].size;
> > +
> > +		if (!ram_size)
> > +			continue;
> > +
> > +		if (ram_start < fdt_ram_start)
> > +			fdt_ram_start = ram_start;
> > +	}
> > +
> > +	/*
> > +	 * Give us at least 12 KiB of breathing room in case the device tree
> > +	 * needs to be expanded later.
> > +	 */
> > +	fdt = *fdtp;
> > +	fdt_pages = efi_size_in_pages(fdt_totalsize(fdt) + 0x3000);
> > +	fdt_size = fdt_pages << EFI_PAGE_SHIFT;
> > +
> > +	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> > +				 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
> > +				 &new_fdt_addr);
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("ERROR: Failed to reserve space for FDT\n");
> > +		goto done;
> > +	}
> > +	new_fdt = (void *)(uintptr_t)new_fdt_addr;
> > +	memcpy(new_fdt, fdt, fdt_totalsize(fdt));
> > +	fdt_set_totalsize(new_fdt, fdt_size);
> > +
> > +	*fdtp = (void *)(uintptr_t)new_fdt_addr;
> > +done:
> > +	return ret;
> > +}
> > +
> > +/**
> > + * get_config_table() - get configuration table
> > + *
> > + * @guid:	GUID of the configuration table
> > + * Return:	pointer to configuration table or NULL
> > + */
> > +static void *get_config_table(const efi_guid_t *guid)
> > +{
> > +	size_t i;
> > +
> > +	for (i = 0; i < systab.nr_tables; i++) {
> > +		if (!guidcmp(guid, &systab.tables[i].guid))
> > +			return systab.tables[i].table;
> > +	}
> > +	return NULL;
> > +}
> > +
> > +#endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
> > +
> > +/**
> > + * efi_install_fdt() - install device tree
> > + *
> > + * If fdt is not EFI_FDT_USE_INTERNAL, the device tree located at that memory
> > + * address will be installed as configuration table, otherwise the device
> > + * tree located at the address indicated by environment variable fdt_addr or as
> > + * fallback fdtcontroladdr will be used.
> > + *
> > + * On architectures using ACPI tables device trees shall not be installed as
> > + * configuration table.
> > + *
> > + * @fdt:	address of device tree or EFI_FDT_USE_INTERNAL to use
> > + *		the hardware device tree as indicated by environment variable
> > + *		fdt_addr or as fallback the internal device tree as indicated by
> > + *		the environment variable fdtcontroladdr
> > + * Return:	status code
> > + */
> > +efi_status_t efi_install_fdt(void *fdt)
> > +{
> > +	/*
> > +	 * The EBBR spec requires that we have either an FDT or an ACPI table
> > +	 * but not both.
> > +	 */
> > +#if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
> > +	if (fdt) {
> > +		log_warning("WARNING: Can't have ACPI table and device tree - ignoring DT.\n");
> > +		return EFI_SUCCESS;
> > +	}
> > +#else
> > +	struct bootm_headers img = { 0 };
> > +	efi_status_t ret;
> > +
> > +	if (fdt == EFI_FDT_USE_INTERNAL) {
> > +		const char *fdt_opt;
> > +		uintptr_t fdt_addr;
> > +
> > +		/* Look for device tree that is already installed */
> > +		if (get_config_table(&efi_guid_fdt))
> > +			return EFI_SUCCESS;
> > +		/* Check if there is a hardware device tree */
> > +		fdt_opt = env_get("fdt_addr");
> > +		/* Use our own device tree as fallback */
> > +		if (!fdt_opt) {
> > +			fdt_opt = env_get("fdtcontroladdr");
> > +			if (!fdt_opt) {
> > +				log_err("ERROR: need device tree\n");
> > +				return EFI_NOT_FOUND;
> > +			}
> > +		}
> > +		fdt_addr = hextoul(fdt_opt, NULL);
> > +		if (!fdt_addr) {
> > +			log_err("ERROR: invalid $fdt_addr or $fdtcontroladdr\n");
> > +			return EFI_LOAD_ERROR;
> > +		}
> > +		fdt = map_sysmem(fdt_addr, 0);
> > +	}
> > +
> > +	/* Install device tree */
> > +	if (fdt_check_header(fdt)) {
> > +		log_err("ERROR: invalid device tree\n");
> > +		return EFI_LOAD_ERROR;
> > +	}
> > +
> > +	/* Prepare device tree for payload */
> > +	ret = copy_fdt(&fdt);
> > +	if (ret) {
> > +		log_err("ERROR: out of memory\n");
> > +		return EFI_OUT_OF_RESOURCES;
> > +	}
> > +
> > +	if (image_setup_libfdt(&img, fdt, NULL)) {
> > +		log_err("ERROR: failed to process device tree\n");
> > +		return EFI_LOAD_ERROR;
> > +	}
> > +
> > +	/* Create memory reservations as indicated by the device tree */
> > +	efi_carve_out_dt_rsv(fdt);
> > +
> > +	efi_try_purge_kaslr_seed(fdt);
> > +
> > +	if (CONFIG_IS_ENABLED(EFI_TCG2_PROTOCOL_MEASURE_DTB)) {
> > +		ret = efi_tcg2_measure_dtb(fdt);
> > +		if (ret == EFI_SECURITY_VIOLATION) {
> > +			log_err("ERROR: failed to measure DTB\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* Install device tree as UEFI table */
> > +	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("ERROR: failed to install device tree\n");
> > +		return ret;
> > +	}
> > +#endif /* GENERATE_ACPI_TABLE */
> > +
> > +	return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * do_bootefi_exec() - execute EFI binary
> > + *
> > + * The image indicated by @handle is started. When it returns the allocated
> > + * memory for the @load_options is freed.
> > + *
> > + * @handle:		handle of loaded image
> > + * @load_options:	load options
> > + * Return:		status code
> > + *
> > + * Load the EFI binary into a newly assigned memory unwinding the relocation
> > + * information, install the loaded image protocol, and call the binary.
> > + */
> > +efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
> > +{
> > +	efi_status_t ret;
> > +	efi_uintn_t exit_data_size = 0;
> > +	u16 *exit_data = NULL;
> > +	struct efi_event *evt;
> > +
> > +	/* On ARM switch from EL3 or secure mode to EL2 or non-secure mode */
> > +	switch_to_non_secure_mode();
> > +
> > +	/*
> > +	 * The UEFI standard requires that the watchdog timer is set to five
> > +	 * minutes when invoking an EFI boot option.
> > +	 *
> > +	 * Unified Extensible Firmware Interface (UEFI), version 2.7 Errata A
> > +	 * 7.5. Miscellaneous Boot Services - EFI_BOOT_SERVICES.SetWatchdogTimer
> > +	 */
> > +	ret = efi_set_watchdog(300);
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("ERROR: Failed to set watchdog timer\n");
> > +		goto out;
> > +	}
> > +
> > +	/* Call our payload! */
> > +	ret = EFI_CALL(efi_start_image(handle, &exit_data_size, &exit_data));
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("## Application failed, r = %lu\n",
> > +			ret & ~EFI_ERROR_MASK);
> > +		if (exit_data) {
> > +			log_err("## %ls\n", exit_data);
> > +			efi_free_pool(exit_data);
> > +		}
> > +	}
> > +
> > +	efi_restore_gd();
> > +
> > +out:
> > +	free(load_options);
> > +
> > +	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
> > +		if (efi_initrd_deregister() != EFI_SUCCESS)
> > +			log_err("Failed to remove loadfile2 for initrd\n");
> > +	}
> > +
> > +	/* Notify EFI_EVENT_GROUP_RETURN_TO_EFIBOOTMGR event group. */
> > +	list_for_each_entry(evt, &efi_events, link) {
> > +		if (evt->group &&
> > +		    !guidcmp(evt->group,
> > +			     &efi_guid_event_group_return_to_efibootmgr)) {
> > +			efi_signal_event(evt);
> > +			EFI_CALL(systab.boottime->close_event(evt));
> > +			break;
> > +		}
> > +	}
> > +
> > +	/* Control is returned to U-Boot, disable EFI watchdog */
> > +	efi_set_watchdog(0);
> > +
> > +	return ret;
> > +}
> > +
> > +#if CONFIG_IS_ENABLED(EFI_BINARY_EXEC)
> > +static struct efi_device_path *bootefi_image_path;
> > +static struct efi_device_path *bootefi_device_path;
> > +static void *image_addr;
> > +static size_t image_size;
> > +
> > +/**
> > + * efi_get_image_parameters() - return image parameters
> > + *
> > + * @img_addr:		address of loaded image in memory
> > + * @img_size:		size of loaded image
> > + */
> > +void efi_get_image_parameters(void **img_addr, size_t *img_size)
> > +{
> > +	*img_addr = image_addr;
> > +	*img_size = image_size;
> > +}
> > +
> > +/**
> > + * efi_clear_bootdev() - clear boot device
> > + */
> > +void efi_clear_bootdev(void)
> > +{
> > +	efi_free_pool(bootefi_device_path);
> > +	efi_free_pool(bootefi_image_path);
> > +	bootefi_device_path = NULL;
> > +	bootefi_image_path = NULL;
> > +	image_addr = NULL;
> > +	image_size = 0;
> > +}
> > +
> > +/**
> > + * efi_set_bootdev() - set boot device
> > + *
> > + * This function is called when a file is loaded, e.g. via the 'load' command.
> > + * We use the path to this file to inform the UEFI binary about the boot device.
> > + *
> > + * @dev:		device, e.g. "MMC"
> > + * @devnr:		number of the device, e.g. "1:2"
> > + * @path:		path to file loaded
> > + * @buffer:		buffer with file loaded
> > + * @buffer_size:	size of file loaded
> > + */
> > +void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
> > +		     void *buffer, size_t buffer_size)
> > +{
> > +	struct efi_device_path *device, *image;
> > +	efi_status_t ret;
> > +
> > +	log_debug("dev=%s, devnr=%s, path=%s, buffer=%p, size=%zx\n", dev,
> > +		  devnr, path, buffer, buffer_size);
> > +
> > +	/* Forget overwritten image */
> > +	if (buffer + buffer_size >= image_addr &&
> > +	    image_addr + image_size >= buffer)
> > +		efi_clear_bootdev();
> > +
> > +	/* Remember only PE-COFF and FIT images */
> > +	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
> > +		if (IS_ENABLED(CONFIG_FIT) &&
> > +		    !fit_check_format(buffer, IMAGE_SIZE_INVAL)) {
> > +			/*
> > +			 * FIT images of type EFI_OS are started via command
> > +			 * bootm. We should not use their boot device with the
> > +			 * bootefi command.
> > +			 */
> > +			buffer = 0;
> > +			buffer_size = 0;
> > +		} else {
> > +			log_debug("- not remembering image\n");
> > +			return;
> > +		}
> > +	}
> > +
> > +	/* efi_set_bootdev() is typically called repeatedly, recover memory */
> > +	efi_clear_bootdev();
> > +
> > +	image_addr = buffer;
> > +	image_size = buffer_size;
> > +
> > +	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> > +	if (ret == EFI_SUCCESS) {
> > +		bootefi_device_path = device;
> > +		if (image) {
> > +			/* FIXME: image should not contain device */
> > +			struct efi_device_path *image_tmp = image;
> > +
> > +			efi_dp_split_file_path(image, &device, &image);
> > +			efi_free_pool(image_tmp);
> > +		}
> > +		bootefi_image_path = image;
> > +		log_debug("- boot device %pD\n", device);
> > +		if (image)
> > +			log_debug("- image %pD\n", image);
> > +	} else {
> > +		log_debug("- efi_dp_from_name() failed, err=%lx\n", ret);
> > +		efi_clear_bootdev();
> > +	}
> > +}
> > +
> > +/**
> > + * efi_env_set_load_options() - set load options from environment variable
> > + *
> > + * @handle:		the image handle
> > + * @env_var:		name of the environment variable
> > + * @load_options:	pointer to load options (output)
> > + * Return:		status code
> > + */
> > +efi_status_t efi_env_set_load_options(efi_handle_t handle,
> > +				      const char *env_var,
> > +				      u16 **load_options)
> > +{
> > +	const char *env = env_get(env_var);
> > +	size_t size;
> > +	u16 *pos;
> > +	efi_status_t ret;
> > +
> > +	*load_options = NULL;
> > +	if (!env)
> > +		return EFI_SUCCESS;
> > +	size = sizeof(u16) * (utf8_utf16_strlen(env) + 1);
> > +	pos = calloc(size, 1);
> > +	if (!pos)
> > +		return EFI_OUT_OF_RESOURCES;
> > +	*load_options = pos;
> > +	utf8_utf16_strcpy(&pos, env);
> > +	ret = efi_set_load_options(handle, size, *load_options);
> > +	if (ret != EFI_SUCCESS) {
> > +		free(*load_options);
> > +		*load_options = NULL;
> > +	}
> > +	return ret;
> > +}
> > +
> > +/**
> > + * efi_run_image() - run loaded UEFI image
> > + *
> > + * @source_buffer:	memory address of the UEFI image
> > + * @source_size:	size of the UEFI image
> > + * Return:		status code
> > + */
> > +efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> > +{
> > +	efi_handle_t mem_handle = NULL, handle;
> > +	struct efi_device_path *file_path = NULL;
> > +	struct efi_device_path *msg_path;
> > +	efi_status_t ret, ret2;
> > +	u16 *load_options;
> > +
> > +	if (!bootefi_device_path || !bootefi_image_path) {
> > +		log_debug("Not loaded from disk\n");
> > +		/*
> > +		 * Special case for efi payload not loaded from disk,
> > +		 * such as 'bootefi hello' or for example payload
> > +		 * loaded directly into memory via JTAG, etc:
> > +		 */
> > +		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> > +					    (uintptr_t)source_buffer,
> > +					    source_size);
> > +		/*
> > +		 * Make sure that device for device_path exist
> > +		 * in load_image(). Otherwise, shell and grub will fail.
> > +		 */
> > +		ret = efi_install_multiple_protocol_interfaces(&mem_handle,
> > +							       &efi_guid_device_path,
> > +							       file_path, NULL);
> > +		if (ret != EFI_SUCCESS)
> > +			goto out;
> > +		msg_path = file_path;
> > +	} else {
> > +		file_path = efi_dp_append(bootefi_device_path,
> > +					  bootefi_image_path);
> > +		msg_path = bootefi_image_path;
> > +		log_debug("Loaded from disk\n");
> > +	}
> > +
> > +	log_info("Booting %pD\n", msg_path);
> > +
> > +	ret = EFI_CALL(efi_load_image(false, efi_root, file_path, source_buffer,
> > +				      source_size, &handle));
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("Loading image failed\n");
> > +		goto out;
> > +	}
> > +
> > +	/* Transfer environment variable as load options */
> > +	ret = efi_env_set_load_options(handle, "bootargs", &load_options);
> > +	if (ret != EFI_SUCCESS)
> > +		goto out;
> > +
> > +	ret = do_bootefi_exec(handle, load_options);
> > +
> > +out:
> > +	ret2 = efi_uninstall_multiple_protocol_interfaces(mem_handle,
> > +							  &efi_guid_device_path,
> > +							  file_path, NULL);
> > +	efi_free_pool(file_path);
> > +	return (ret != EFI_SUCCESS) ? ret : ret2;
> > +}
> > +
> > +/**
> > + * efi_binary_run() - run loaded UEFI image
> > + *
> > + * @image:	memory address of the UEFI image
> > + * @size:	size of the UEFI image
> > + * @fdt:	device-tree
> > + *
> > + * Execute an EFI binary image loaded at @image.
> > + * @size may be zero if the binary is loaded with U-Boot load command.
> > + *
> > + * Return:	status code
> > + */
> > +efi_status_t efi_binary_run(void *image, size_t size, void *fdt)
> > +{
> > +	efi_status_t ret;
> > +
> > +	/* Initialize EFI drivers */
> > +	ret = efi_init_obj_list();
> > +	if (ret != EFI_SUCCESS) {
> > +		log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> > +			ret & ~EFI_ERROR_MASK);
> > +		return -1;
> > +	}
> > +
> > +	ret = efi_install_fdt(fdt);
> > +	if (ret != EFI_SUCCESS)
> > +		return ret;
> > +
> > +	return efi_run_image(image, size);
> > +}
> > +#endif /* CONFIG_BINARY_EXEC */
> 

[PATCH v3 2/4] efi_loader: rename BOOTEFI_BOOTMGR to EFI_BOOTMGR

[AKASHI Takahiro]

At this point, EFI boot manager interfaces is fully independent from
bootefi command. So just rename the configuration parameter.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 boot/Makefile           | 2 +-
 cmd/Kconfig             | 4 ++--
 cmd/efidebug.c          | 4 ++--
 lib/efi_loader/Kconfig  | 2 +-
 lib/efi_loader/Makefile | 2 +-
 test/boot/bootflow.c    | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/boot/Makefile b/boot/Makefile
index a90ebea5a867..bcd01cfc890c 100644
--- a/boot/Makefile
+++ b/boot/Makefile
@@ -34,7 +34,7 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_CROS) += bootm.o bootm_os.o bootmeth_cros.o
 obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o
 obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SCRIPT) += bootmeth_script.o
 ifdef CONFIG_$(SPL_TPL_)BOOTSTD_FULL
-obj-$(CONFIG_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o
+obj-$(CONFIG_EFI_BOOTMGR) += bootmeth_efi_mgr.o
 obj-$(CONFIG_$(SPL_TPL_)EXPO) += bootflow_menu.o
 obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootflow_menu.o
 obj-$(CONFIG_$(SPL_TPL_)CEDIT) += cedit.o
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 2c993496b70e..856ff9f5395d 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -374,7 +374,7 @@ config CMD_BOOTEFI_BINARY
 
 config CMD_BOOTEFI_BOOTMGR
 	bool "UEFI Boot Manager command"
-	depends on BOOTEFI_BOOTMGR
+	depends on EFI_BOOTMGR
 	default y
 	help
 	  Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
@@ -2122,7 +2122,7 @@ config CMD_EFIDEBUG
 config CMD_EFICONFIG
 	bool "eficonfig - provide menu-driven uefi variables maintenance interface"
 	default y if !HAS_BOARD_SIZE_LIMIT
-	depends on BOOTEFI_BOOTMGR
+	depends on EFI_BOOTMGR
 	select MENU
 	help
 	  Enable the 'eficonfig' command which provides the menu-driven UEFI
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index e10fbf891a42..b4954258eeba 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -1410,7 +1410,7 @@ static __maybe_unused int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
 }
 
 static struct cmd_tbl cmd_efidebug_test_sub[] = {
-#ifdef CONFIG_BOOTEFI_BOOTMGR
+#ifdef CONFIG_EFI_BOOTMGR
 	U_BOOT_CMD_MKENT(bootmgr, CONFIG_SYS_MAXARGS, 1, do_efi_test_bootmgr,
 			 "", ""),
 #endif
@@ -1604,7 +1604,7 @@ U_BOOT_LONGHELP(efidebug,
 	"  - show UEFI memory map\n"
 	"efidebug tables\n"
 	"  - show UEFI configuration tables\n"
-#ifdef CONFIG_BOOTEFI_BOOTMGR
+#ifdef CONFIG_EFI_BOOTMGR
 	"efidebug test bootmgr\n"
 	"  - run simple bootmgr for test\n"
 #endif
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 64f2f1cdd161..db5571de1d95 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -41,7 +41,7 @@ config EFI_BINARY_EXEC
 	  You may enable CMD_BOOTEFI_BINARY so that you can use bootefi
 	  command to do that.
 
-config BOOTEFI_BOOTMGR
+config EFI_BOOTMGR
 	bool "UEFI Boot Manager"
 	default y
 	select BOOTMETH_GLOBAL if BOOTSTD
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 0a2cb6e3c476..f882474bba6f 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -42,7 +42,7 @@ targets += initrddump.o
 endif
 
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
-obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
+obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o
 obj-y += efi_boottime.o
 obj-y += efi_helper.o
 obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index f3e5a839da47..57e48fe62694 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -374,7 +374,7 @@ static int bootflow_system(struct unit_test_state *uts)
 {
 	struct udevice *bootstd, *dev;
 
-	if (!IS_ENABLED(CONFIG_BOOTEFI_BOOTMGR))
+	if (!IS_ENABLED(CONFIG_EFI_BOOTMGR))
 		return -EAGAIN;
 	ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd));
 	ut_assertok(device_bind(bootstd, DM_DRIVER_GET(bootmeth_efi_mgr),
-- 
2.34.1

Re: [PATCH v3 2/4] efi_loader: rename BOOTEFI_BOOTMGR to EFI_BOOTMGR

[Simon Glass]

On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> At this point, EFI boot manager interfaces is fully independent from
> bootefi command. So just rename the configuration parameter.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  boot/Makefile           | 2 +-
>  cmd/Kconfig             | 4 ++--
>  cmd/efidebug.c          | 4 ++--
>  lib/efi_loader/Kconfig  | 2 +-
>  lib/efi_loader/Makefile | 2 +-
>  test/boot/bootflow.c    | 2 +-
>  6 files changed, 8 insertions(+), 8 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[AKASHI Takahiro]

Now it is clear that the feature actually depends on efi interfaces,
not "bootefi" command. efi_set_bootdev() will automatically be nullified
if necessary efi component is disabled.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 net/tftp.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/tftp.c b/net/tftp.c
index 88e71e67de35..2e335413492b 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -302,12 +302,10 @@ static void tftp_complete(void)
 			time_start * 1000, "/s");
 	}
 	puts("\ndone\n");
-	if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
-		if (!tftp_put_active)
-			efi_set_bootdev("Net", "", tftp_filename,
-					map_sysmem(tftp_load_addr, 0),
-					net_boot_file_size);
-	}
+	if (!tftp_put_active)
+		efi_set_bootdev("Net", "", tftp_filename,
+				map_sysmem(tftp_load_addr, 0),
+				net_boot_file_size);
 	net_set_state(NETLOOP_SUCCESS);
 }
 
-- 
2.34.1

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[Ramon Fried]

On Mon, Dec 18, 2023 at 5:17 AM AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Now it is clear that the feature actually depends on efi interfaces,
> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> if necessary efi component is disabled.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  net/tftp.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/net/tftp.c b/net/tftp.c
> index 88e71e67de35..2e335413492b 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -302,12 +302,10 @@ static void tftp_complete(void)
>                         time_start * 1000, "/s");
>         }
>         puts("\ndone\n");
> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
> -               if (!tftp_put_active)
> -                       efi_set_bootdev("Net", "", tftp_filename,
> -                                       map_sysmem(tftp_load_addr, 0),
> -                                       net_boot_file_size);
> -       }
> +       if (!tftp_put_active)
> +               efi_set_bootdev("Net", "", tftp_filename,
> +                               map_sysmem(tftp_load_addr, 0),
> +                               net_boot_file_size);
>         net_set_state(NETLOOP_SUCCESS);
>  }
>
> --
> 2.34.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[Simon Glass]

Hi AKASHI,

On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Now it is clear that the feature actually depends on efi interfaces,
> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> if necessary efi component is disabled.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  net/tftp.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>

I have the same comment here as the 'fs' patch.

> diff --git a/net/tftp.c b/net/tftp.c
> index 88e71e67de35..2e335413492b 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -302,12 +302,10 @@ static void tftp_complete(void)
>                         time_start * 1000, "/s");
>         }
>         puts("\ndone\n");
> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {

Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER
is not enabled?

> -               if (!tftp_put_active)
> -                       efi_set_bootdev("Net", "", tftp_filename,
> -                                       map_sysmem(tftp_load_addr, 0),
> -                                       net_boot_file_size);
> -       }
> +       if (!tftp_put_active)
> +               efi_set_bootdev("Net", "", tftp_filename,
> +                               map_sysmem(tftp_load_addr, 0),
> +                               net_boot_file_size);
>         net_set_state(NETLOOP_SUCCESS);
>  }
>
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[AKASHI Takahiro]

Hi Simon,

On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote:
> Hi AKASHI,
> 
> On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Now it is clear that the feature actually depends on efi interfaces,
> > not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > if necessary efi component is disabled.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  net/tftp.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> 
> I have the same comment here as the 'fs' patch.
> 
> > diff --git a/net/tftp.c b/net/tftp.c
> > index 88e71e67de35..2e335413492b 100644
> > --- a/net/tftp.c
> > +++ b/net/tftp.c
> > @@ -302,12 +302,10 @@ static void tftp_complete(void)
> >                         time_start * 1000, "/s");
> >         }
> >         puts("\ndone\n");
> > -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
> 
> Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER
> is not enabled?

The trick is in efi_loader.h.
If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined,
this function gets voided.  See patch#1 in this version.

I took this approach in order not to make users much worried about
what config be used as they are not familiar with UEFI implementation.

-Takahiro Akashi

> > -               if (!tftp_put_active)
> > -                       efi_set_bootdev("Net", "", tftp_filename,
> > -                                       map_sysmem(tftp_load_addr, 0),
> > -                                       net_boot_file_size);
> > -       }
> > +       if (!tftp_put_active)
> > +               efi_set_bootdev("Net", "", tftp_filename,
> > +                               map_sysmem(tftp_load_addr, 0),
> > +                               net_boot_file_size);
> >         net_set_state(NETLOOP_SUCCESS);
> >  }
> >
> > --
> > 2.34.1
> >
> 
> Regards,
> Simon

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[Simon Glass]

Hi,

On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote:
> > Hi AKASHI,
> >
> > On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > Now it is clear that the feature actually depends on efi interfaces,
> > > not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > > if necessary efi component is disabled.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >  net/tftp.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> >
> > I have the same comment here as the 'fs' patch.
> >
> > > diff --git a/net/tftp.c b/net/tftp.c
> > > index 88e71e67de35..2e335413492b 100644
> > > --- a/net/tftp.c
> > > +++ b/net/tftp.c
> > > @@ -302,12 +302,10 @@ static void tftp_complete(void)
> > >                         time_start * 1000, "/s");
> > >         }
> > >         puts("\ndone\n");
> > > -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
> >
> > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER
> > is not enabled?
>
> The trick is in efi_loader.h.
> If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined,
> this function gets voided.  See patch#1 in this version.
>
> I took this approach in order not to make users much worried about
> what config be used as they are not familiar with UEFI implementation.

OK, but we really need to delete this function, so what is the plan
for that? The info that EFI needs should be passed to the bootefi()
function, not set in a global.


>
> -Takahiro Akashi
>
> > > -               if (!tftp_put_active)
> > > -                       efi_set_bootdev("Net", "", tftp_filename,
> > > -                                       map_sysmem(tftp_load_addr, 0),
> > > -                                       net_boot_file_size);
> > > -       }
> > > +       if (!tftp_put_active)
> > > +               efi_set_bootdev("Net", "", tftp_filename,
> > > +                               map_sysmem(tftp_load_addr, 0),
> > > +                               net_boot_file_size);
> > >         net_set_state(NETLOOP_SUCCESS);
> > >  }
> > >
> > > --
> > > 2.34.1

Regards,
Simon

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[Heinrich Schuchardt]

Am 20. Dezember 2023 05:46:16 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi,
>
>On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro
><takahiro.akashi@linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote:
>> > Hi AKASHI,
>> >
>> > On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
>> > <takahiro.akashi@linaro.org> wrote:
>> > >
>> > > Now it is clear that the feature actually depends on efi interfaces,
>> > > not "bootefi" command. efi_set_bootdev() will automatically be nullified
>> > > if necessary efi component is disabled.
>> > >
>> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> > > ---
>> > >  net/tftp.c | 10 ++++------
>> > >  1 file changed, 4 insertions(+), 6 deletions(-)
>> > >
>> >
>> > I have the same comment here as the 'fs' patch.
>> >
>> > > diff --git a/net/tftp.c b/net/tftp.c
>> > > index 88e71e67de35..2e335413492b 100644
>> > > --- a/net/tftp.c
>> > > +++ b/net/tftp.c
>> > > @@ -302,12 +302,10 @@ static void tftp_complete(void)
>> > >                         time_start * 1000, "/s");
>> > >         }
>> > >         puts("\ndone\n");
>> > > -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
>> >
>> > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER
>> > is not enabled?
>>
>> The trick is in efi_loader.h.
>> If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined,
>> this function gets voided.  See patch#1 in this version.
>>
>> I took this approach in order not to make users much worried about
>> what config be used as they are not familiar with UEFI implementation.
>
>OK, but we really need to delete this function, so what is the plan
>for that? The info that EFI needs should be passed to the bootefi()
>function, not set in a global.

Hello Simon,

Bootstd is not the only way to boot. Please, do not forget the shell.

The user loads a file with tftpboot. At some later moment the user executes bootefi.

We need a place where we store the device from which the image was loaded.

In future we might have a register of loaded files. But that is beyond the scope of this patch series.

Best regards

Heinrich

>
>
>>
>> -Takahiro Akashi
>>
>> > > -               if (!tftp_put_active)
>> > > -                       efi_set_bootdev("Net", "", tftp_filename,
>> > > -                                       map_sysmem(tftp_load_addr, 0),
>> > > -                                       net_boot_file_size);
>> > > -       }
>> > > +       if (!tftp_put_active)
>> > > +               efi_set_bootdev("Net", "", tftp_filename,
>> > > +                               map_sysmem(tftp_load_addr, 0),
>> > > +                               net_boot_file_size);
>> > >         net_set_state(NETLOOP_SUCCESS);
>> > >  }
>> > >
>> > > --
>> > > 2.34.1
>
>Regards,
>Simon

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[Simon Glass]

Hi Heinrich,

On Wed, Dec 20, 2023 at 9:17 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 20. Dezember 2023 05:46:16 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi,
> >
> >On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro
> ><takahiro.akashi@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote:
> >> > Hi AKASHI,
> >> >
> >> > On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> >> > <takahiro.akashi@linaro.org> wrote:
> >> > >
> >> > > Now it is clear that the feature actually depends on efi interfaces,
> >> > > not "bootefi" command. efi_set_bootdev() will automatically be nullified
> >> > > if necessary efi component is disabled.
> >> > >
> >> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> > > ---
> >> > >  net/tftp.c | 10 ++++------
> >> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> >> > >
> >> >
> >> > I have the same comment here as the 'fs' patch.
> >> >
> >> > > diff --git a/net/tftp.c b/net/tftp.c
> >> > > index 88e71e67de35..2e335413492b 100644
> >> > > --- a/net/tftp.c
> >> > > +++ b/net/tftp.c
> >> > > @@ -302,12 +302,10 @@ static void tftp_complete(void)
> >> > >                         time_start * 1000, "/s");
> >> > >         }
> >> > >         puts("\ndone\n");
> >> > > -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
> >> >
> >> > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER
> >> > is not enabled?
> >>
> >> The trick is in efi_loader.h.
> >> If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined,
> >> this function gets voided.  See patch#1 in this version.
> >>
> >> I took this approach in order not to make users much worried about
> >> what config be used as they are not familiar with UEFI implementation.
> >
> >OK, but we really need to delete this function, so what is the plan
> >for that? The info that EFI needs should be passed to the bootefi()
> >function, not set in a global.
>
> Hello Simon,
>
> Bootstd is not the only way to boot. Please, do not forget the shell.
>
> The user loads a file with tftpboot. At some later moment the user executes bootefi.
>
> We need a place where we store the device from which the image was loaded.

Yes, agreed. See my other reply on that.

>
> In future we might have a register of loaded files. But that is beyond the scope of this patch series.

I believe we could just record the device and partition number (which
is itself a device these days, I suppose). Then for EFI can do the
translation at the start of the bootm cmd if not using bootstd.

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> Best regards
>
> Heinrich
>
> >
> >
> >>
> >> -Takahiro Akashi
> >>
> >> > > -               if (!tftp_put_active)
> >> > > -                       efi_set_bootdev("Net", "", tftp_filename,
> >> > > -                                       map_sysmem(tftp_load_addr, 0),
> >> > > -                                       net_boot_file_size);
> >> > > -       }
> >> > > +       if (!tftp_put_active)
> >> > > +               efi_set_bootdev("Net", "", tftp_filename,
> >> > > +                               map_sysmem(tftp_load_addr, 0),
> >> > > +                               net_boot_file_size);
> >> > >         net_set_state(NETLOOP_SUCCESS);
> >> > >  }
> >> > >
> >> > > --
> >> > > 2.34.1
> >
> >Regards,

Regards,
Simon

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[AKASHI Takahiro]

Hi Simon,

On Tue, Dec 26, 2023 at 09:47:03AM +0000, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, Dec 20, 2023 at 9:17???AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > Am 20. Dezember 2023 05:46:16 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > >Hi,
> > >
> > >On Mon, 18 Dec 2023 at 17:17, AKASHI Takahiro
> > ><takahiro.akashi@linaro.org> wrote:
> > >>
> > >> Hi Simon,
> > >>
> > >> On Mon, Dec 18, 2023 at 08:01:46AM -0700, Simon Glass wrote:
> > >> > Hi AKASHI,
> > >> >
> > >> > On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> > >> > <takahiro.akashi@linaro.org> wrote:
> > >> > >
> > >> > > Now it is clear that the feature actually depends on efi interfaces,
> > >> > > not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > >> > > if necessary efi component is disabled.
> > >> > >
> > >> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >> > > ---
> > >> > >  net/tftp.c | 10 ++++------
> > >> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > >> > >
> > >> >
> > >> > I have the same comment here as the 'fs' patch.
> > >> >
> > >> > > diff --git a/net/tftp.c b/net/tftp.c
> > >> > > index 88e71e67de35..2e335413492b 100644
> > >> > > --- a/net/tftp.c
> > >> > > +++ b/net/tftp.c
> > >> > > @@ -302,12 +302,10 @@ static void tftp_complete(void)
> > >> > >                         time_start * 1000, "/s");
> > >> > >         }
> > >> > >         puts("\ndone\n");
> > >> > > -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
> > >> >
> > >> > Shouldn't this depend on your new CONFIG? What happens if EFI_LOADER
> > >> > is not enabled?
> > >>
> > >> The trick is in efi_loader.h.
> > >> If EFI_LOADER (more specifically CONFIG_EFI_BINARY_EXEC) is not defined,
> > >> this function gets voided.  See patch#1 in this version.
> > >>
> > >> I took this approach in order not to make users much worried about
> > >> what config be used as they are not familiar with UEFI implementation.
> > >
> > >OK, but we really need to delete this function, so what is the plan
> > >for that? The info that EFI needs should be passed to the bootefi()
> > >function, not set in a global.
> >
> > Hello Simon,
> >
> > Bootstd is not the only way to boot. Please, do not forget the shell.
> >
> > The user loads a file with tftpboot. At some later moment the user executes bootefi.
> >
> > We need a place where we store the device from which the image was loaded.
> 
> Yes, agreed. See my other reply on that.
> 
> >
> > In future we might have a register of loaded files. But that is beyond the scope of this patch series.
> 
> I believe we could just record the device and partition number (which
> is itself a device these days, I suppose). Then for EFI can do the
> translation at the start of the bootm cmd if not using bootstd.

Then, what is the difference between "record the device and partition
number (strictly it's not accurate because we also support tftp and others.)
and "call efi_set_bootdev()"?

The latter does the translation on the fly and saves the info
in bootefi_image_path and bootefi_device_path local variables.

I believe that both are essentially the same.

> Reviewed-by: Simon Glass <sjg@chromium.org>

Always thank you for your review.

-Takahiro Akashi

> 
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > >
> > >>
> > >> -Takahiro Akashi
> > >>
> > >> > > -               if (!tftp_put_active)
> > >> > > -                       efi_set_bootdev("Net", "", tftp_filename,
> > >> > > -                                       map_sysmem(tftp_load_addr, 0),
> > >> > > -                                       net_boot_file_size);
> > >> > > -       }
> > >> > > +       if (!tftp_put_active)
> > >> > > +               efi_set_bootdev("Net", "", tftp_filename,
> > >> > > +                               map_sysmem(tftp_load_addr, 0),
> > >> > > +                               net_boot_file_size);
> > >> > >         net_set_state(NETLOOP_SUCCESS);
> > >> > >  }
> > >> > >
> > >> > > --
> > >> > > 2.34.1
> > >
> > >Regards,
> 
> Regards,
> Simon

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[Heinrich Schuchardt]

On 12/18/23 03:38, AKASHI Takahiro wrote:
> Now it is clear that the feature actually depends on efi interfaces,
> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> if necessary efi component is disabled.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   net/tftp.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/net/tftp.c b/net/tftp.c
> index 88e71e67de35..2e335413492b 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -302,12 +302,10 @@ static void tftp_complete(void)
>   			time_start * 1000, "/s");
>   	}
>   	puts("\ndone\n");
> -	if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
> -		if (!tftp_put_active)
> -			efi_set_bootdev("Net", "", tftp_filename,
> -					map_sysmem(tftp_load_addr, 0),
> -					net_boot_file_size);
> -	}
> +	if (!tftp_put_active)
> +		efi_set_bootdev("Net", "", tftp_filename,

Function efi_set_bootdev() will not be compiled for CONFIG_EFI_LOADER=n.
So this change may lead to build failures.

Best regards

Heinrich

> +				map_sysmem(tftp_load_addr, 0),
> +				net_boot_file_size);
>   	net_set_state(NETLOOP_SUCCESS);
>   }
>

Re: [PATCH v3 3/4] net: tftp: remove explicit efi configuration dependency

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

On Mon, Dec 25, 2023 at 10:23:35AM +0100, Heinrich Schuchardt wrote:
> On 12/18/23 03:38, AKASHI Takahiro wrote:
> > Now it is clear that the feature actually depends on efi interfaces,
> > not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > if necessary efi component is disabled.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   net/tftp.c | 10 ++++------
> >   1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/tftp.c b/net/tftp.c
> > index 88e71e67de35..2e335413492b 100644
> > --- a/net/tftp.c
> > +++ b/net/tftp.c
> > @@ -302,12 +302,10 @@ static void tftp_complete(void)
> >   			time_start * 1000, "/s");
> >   	}
> >   	puts("\ndone\n");
> > -	if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
> > -		if (!tftp_put_active)
> > -			efi_set_bootdev("Net", "", tftp_filename,
> > -					map_sysmem(tftp_load_addr, 0),
> > -					net_boot_file_size);
> > -	}
> > +	if (!tftp_put_active)
> > +		efi_set_bootdev("Net", "", tftp_filename,
> 
> Function efi_set_bootdev() will not be compiled for CONFIG_EFI_LOADER=n.
> So this change may lead to build failures.

In the EFI_LOADER=n case <efi_loader.h> has an empty inline instead so
this should be fine.

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v3 4/4] fs: remove explicit efi configuration dependency

[AKASHI Takahiro]

Now it is clear that the feature actually depends on efi interfaces,
not "bootefi" command. efi_set_bootdev() will automatically be nullified
if necessary efi component is disabled.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fs.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index f33b85f92b61..82ee03b160e9 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
 		return 1;
 	}
 
-	if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
-		efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
-				(argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
-				len_read);
+	efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
+			(argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
+			len_read);
 
 	printf("%llu bytes read in %lu ms", len_read, time);
 	if (time > 0) {
-- 
2.34.1

Re: [PATCH v3 4/4] fs: remove explicit efi configuration dependency

[Simon Glass]

Hi AKASHI,

On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Now it is clear that the feature actually depends on efi interfaces,
> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> if necessary efi component is disabled.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  fs/fs.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index f33b85f92b61..82ee03b160e9 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
>                 return 1;
>         }
>
> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
> -               efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> -                               (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> -                               len_read);
> +       efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> +                       (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> +                       len_read);

As I understand it, this is setting the boot device so that (if it
happens to be an efi application) it will know which device it came
from. But this is a hack. For bootstd, the device is known as it loads
the kernel.

Also it does not deal with memory allocation (nor can it).

Where are we using the 'load' command to load a kernel? The distro
scripts are deprecated.

At some point this code should be removed. Is it too early for that?

>
>         printf("%llu bytes read in %lu ms", len_read, time);
>         if (time > 0) {
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v3 4/4] fs: remove explicit efi configuration dependency

[Heinrich Schuchardt]

Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi AKASHI,
>
>On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
><takahiro.akashi@linaro.org> wrote:
>>
>> Now it is clear that the feature actually depends on efi interfaces,
>> not "bootefi" command. efi_set_bootdev() will automatically be nullified
>> if necessary efi component is disabled.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  fs/fs.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/fs.c b/fs/fs.c
>> index f33b85f92b61..82ee03b160e9 100644
>> --- a/fs/fs.c
>> +++ b/fs/fs.c
>> @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
>>                 return 1;
>>         }
>>
>> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
>> -               efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
>> -                               (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
>> -                               len_read);
>> +       efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
>> +                       (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
>> +                       len_read);
>
>As I understand it, this is setting the boot device so that (if it
>happens to be an efi application) it will know which device it came
>from. But this is a hack. For bootstd, the device is known as it loads
>the kernel.

Please, consider what happens when the user interactively executes the load and bootefi command from the console.

>
>Also it does not deal with memory allocation (nor can it).
>
>Where are we using the 'load' command to load a kernel? The distro
>scripts are deprecated.

Some users use boot.scr scripts 


>
>At some point this code should be removed. Is it too early for that?

Yes, as long as we allow users to invoke the bootefi command with an address pointer.

Best regards

Heinrich

>
>>
>>         printf("%llu bytes read in %lu ms", len_read, time);
>>         if (time > 0) {
>> --
>> 2.34.1
>>
>
>Regards,
>Simon

Re: [PATCH v3 4/4] fs: remove explicit efi configuration dependency

[Simon Glass]

Hi Heinrich,

On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi AKASHI,
> >
> >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> ><takahiro.akashi@linaro.org> wrote:
> >>
> >> Now it is clear that the feature actually depends on efi interfaces,
> >> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> >> if necessary efi component is disabled.
> >>
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> ---
> >>  fs/fs.c | 7 +++----
> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/fs.c b/fs/fs.c
> >> index f33b85f92b61..82ee03b160e9 100644
> >> --- a/fs/fs.c
> >> +++ b/fs/fs.c
> >> @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
> >>                 return 1;
> >>         }
> >>
> >> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
> >> -               efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> >> -                               (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> >> -                               len_read);
> >> +       efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> >> +                       (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> >> +                       len_read);
> >
> >As I understand it, this is setting the boot device so that (if it
> >happens to be an efi application) it will know which device it came
> >from. But this is a hack. For bootstd, the device is known as it loads
> >the kernel.
>
> Please, consider what happens when the user interactively executes the load and bootefi command from the console.
>
> >
> >Also it does not deal with memory allocation (nor can it).
> >
> >Where are we using the 'load' command to load a kernel? The distro
> >scripts are deprecated.
>
> Some users use boot.scr scripts
>
>
> >
> >At some point this code should be removed. Is it too early for that?
>
> Yes, as long as we allow users to invoke the bootefi command with an address pointer.

So how about we create the new path, with the info passed in as part
of the bootefi call? Then we can remove the call from bootstd, at
least.

Regards,
Simon

Re: [PATCH v3 4/4] fs: remove explicit efi configuration dependency

[Heinrich Schuchardt]

Am 20. Dezember 2023 05:46:27 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Heinrich,
>
>On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>:
>> >Hi AKASHI,
>> >
>> >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
>> ><takahiro.akashi@linaro.org> wrote:
>> >>
>> >> Now it is clear that the feature actually depends on efi interfaces,
>> >> not "bootefi" command. efi_set_bootdev() will automatically be nullified
>> >> if necessary efi component is disabled.
>> >>
>> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> >> ---
>> >>  fs/fs.c | 7 +++----
>> >>  1 file changed, 3 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/fs/fs.c b/fs/fs.c
>> >> index f33b85f92b61..82ee03b160e9 100644
>> >> --- a/fs/fs.c
>> >> +++ b/fs/fs.c
>> >> @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
>> >>                 return 1;
>> >>         }
>> >>
>> >> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
>> >> -               efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
>> >> -                               (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
>> >> -                               len_read);
>> >> +       efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
>> >> +                       (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
>> >> +                       len_read);
>> >
>> >As I understand it, this is setting the boot device so that (if it
>> >happens to be an efi application) it will know which device it came
>> >from. But this is a hack. For bootstd, the device is known as it loads
>> >the kernel.
>>
>> Please, consider what happens when the user interactively executes the load and bootefi command from the console.
>>
>> >
>> >Also it does not deal with memory allocation (nor can it).
>> >
>> >Where are we using the 'load' command to load a kernel? The distro
>> >scripts are deprecated.
>>
>> Some users use boot.scr scripts
>>
>>
>> >
>> >At some point this code should be removed. Is it too early for that?
>>
>> Yes, as long as we allow users to invoke the bootefi command with an address pointer.
>
>So how about we create the new path, with the info passed in as part
>of the bootefi call? Then we can remove the call from bootstd, at
>least.

Hello Simon,

Do you see a problem merging the current patch? Or are you talking about what we might do after this patch?

Best regards

Heinrich

Re: [PATCH v3 4/4] fs: remove explicit efi configuration dependency

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

On Tue, Dec 19, 2023 at 09:46:27PM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > >Hi AKASHI,
> > >
> > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> > ><takahiro.akashi@linaro.org> wrote:
> > >>
> > >> Now it is clear that the feature actually depends on efi interfaces,
> > >> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > >> if necessary efi component is disabled.
> > >>
> > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >> ---
> > >>  fs/fs.c | 7 +++----
> > >>  1 file changed, 3 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/fs/fs.c b/fs/fs.c
> > >> index f33b85f92b61..82ee03b160e9 100644
> > >> --- a/fs/fs.c
> > >> +++ b/fs/fs.c
> > >> @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
> > >>                 return 1;
> > >>         }
> > >>
> > >> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
> > >> -               efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > >> -                               (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > >> -                               len_read);
> > >> +       efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > >> +                       (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > >> +                       len_read);
> > >
> > >As I understand it, this is setting the boot device so that (if it
> > >happens to be an efi application) it will know which device it came
> > >from. But this is a hack. For bootstd, the device is known as it loads
> > >the kernel.
> >
> > Please, consider what happens when the user interactively executes the load and bootefi command from the console.
> >
> > >
> > >Also it does not deal with memory allocation (nor can it).
> > >
> > >Where are we using the 'load' command to load a kernel? The distro
> > >scripts are deprecated.
> >
> > Some users use boot.scr scripts
> >
> >
> > >
> > >At some point this code should be removed. Is it too early for that?
> >
> > Yes, as long as we allow users to invoke the bootefi command with an address pointer.
> 
> So how about we create the new path, with the info passed in as part
> of the bootefi call? Then we can remove the call from bootstd, at
> least.

Why? It's not clear to me we'd have this information available at that
point unless we stored things to pass along at that point too.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 4/4] fs: remove explicit efi configuration dependency

[Simon Glass]

Hi,

On Wed, Dec 20, 2023 at 1:25 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Dec 19, 2023 at 09:46:27PM -0700, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > > >Hi AKASHI,
> > > >
> > > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> > > ><takahiro.akashi@linaro.org> wrote:
> > > >>
> > > >> Now it is clear that the feature actually depends on efi interfaces,
> > > >> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > > >> if necessary efi component is disabled.
> > > >>
> > > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > >> ---
> > > >>  fs/fs.c | 7 +++----
> > > >>  1 file changed, 3 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/fs/fs.c b/fs/fs.c
> > > >> index f33b85f92b61..82ee03b160e9 100644
> > > >> --- a/fs/fs.c
> > > >> +++ b/fs/fs.c
> > > >> @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
> > > >>                 return 1;
> > > >>         }
> > > >>
> > > >> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
> > > >> -               efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > > >> -                               (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > > >> -                               len_read);
> > > >> +       efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > > >> +                       (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > > >> +                       len_read);
> > > >
> > > >As I understand it, this is setting the boot device so that (if it
> > > >happens to be an efi application) it will know which device it came
> > > >from. But this is a hack. For bootstd, the device is known as it loads
> > > >the kernel.
> > >
> > > Please, consider what happens when the user interactively executes the load and bootefi command from the console.

Yes, that case needs to be handled.

> > >
> > > >
> > > >Also it does not deal with memory allocation (nor can it).
> > > >
> > > >Where are we using the 'load' command to load a kernel? The distro
> > > >scripts are deprecated.
> > >
> > > Some users use boot.scr scripts

We will need to figure out that. But we should make bootstd do the right thing.

> > >
> > >
> > > >
> > > >At some point this code should be removed. Is it too early for that?
> > >
> > > Yes, as long as we allow users to invoke the bootefi command with an address pointer.
> >
> > So how about we create the new path, with the info passed in as part
> > of the bootefi call? Then we can remove the call from bootstd, at
> > least.
>
> Why? It's not clear to me we'd have this information available at that
> point unless we stored things to pass along at that point too.

For the bootstd case, we know the device where the kernel came from.
See distro_efi_boot() for this code. It calls efiload_read_file()
which calls set_efi_bootdev().

We should instead pass the device information to efi_binary_run().

With that done, we can perhaps make the 'load' and 'tftp' commands
record the device information somewhere, so that bootm can pick it up
when it starts and put it in 'images'. Then at some point we can clean
up the 'devnr' parameter, which should really be a partition number,
since the 'dev' parameter tells you the device.

Anyway, this series is fine, so long as we can agree to the above.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH v3 4/4] fs: remove explicit efi configuration dependency

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4419 bytes --]

On Tue, Dec 26, 2023 at 09:46:52AM +0000, Simon Glass wrote:
> Hi,
> 
> On Wed, Dec 20, 2023 at 1:25 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Tue, Dec 19, 2023 at 09:46:27PM -0700, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Mon, 18 Dec 2023 at 15:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > >
> > > >
> > > > Am 18. Dezember 2023 16:01:40 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > > > >Hi AKASHI,
> > > > >
> > > > >On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
> > > > ><takahiro.akashi@linaro.org> wrote:
> > > > >>
> > > > >> Now it is clear that the feature actually depends on efi interfaces,
> > > > >> not "bootefi" command. efi_set_bootdev() will automatically be nullified
> > > > >> if necessary efi component is disabled.
> > > > >>
> > > > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > >> ---
> > > > >>  fs/fs.c | 7 +++----
> > > > >>  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > >>
> > > > >> diff --git a/fs/fs.c b/fs/fs.c
> > > > >> index f33b85f92b61..82ee03b160e9 100644
> > > > >> --- a/fs/fs.c
> > > > >> +++ b/fs/fs.c
> > > > >> @@ -791,10 +791,9 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],
> > > > >>                 return 1;
> > > > >>         }
> > > > >>
> > > > >> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
> > > > >> -               efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > > > >> -                               (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > > > >> -                               len_read);
> > > > >> +       efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> > > > >> +                       (argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
> > > > >> +                       len_read);
> > > > >
> > > > >As I understand it, this is setting the boot device so that (if it
> > > > >happens to be an efi application) it will know which device it came
> > > > >from. But this is a hack. For bootstd, the device is known as it loads
> > > > >the kernel.
> > > >
> > > > Please, consider what happens when the user interactively executes the load and bootefi command from the console.
> 
> Yes, that case needs to be handled.
> 
> > > >
> > > > >
> > > > >Also it does not deal with memory allocation (nor can it).
> > > > >
> > > > >Where are we using the 'load' command to load a kernel? The distro
> > > > >scripts are deprecated.
> > > >
> > > > Some users use boot.scr scripts
> 
> We will need to figure out that. But we should make bootstd do the right thing.
> 
> > > >
> > > >
> > > > >
> > > > >At some point this code should be removed. Is it too early for that?
> > > >
> > > > Yes, as long as we allow users to invoke the bootefi command with an address pointer.
> > >
> > > So how about we create the new path, with the info passed in as part
> > > of the bootefi call? Then we can remove the call from bootstd, at
> > > least.
> >
> > Why? It's not clear to me we'd have this information available at that
> > point unless we stored things to pass along at that point too.
> 
> For the bootstd case, we know the device where the kernel came from.
> See distro_efi_boot() for this code. It calls efiload_read_file()
> which calls set_efi_bootdev().
> 
> We should instead pass the device information to efi_binary_run().
> 
> With that done, we can perhaps make the 'load' and 'tftp' commands
> record the device information somewhere, so that bootm can pick it up
> when it starts and put it in 'images'. Then at some point we can clean
> up the 'devnr' parameter, which should really be a partition number,
> since the 'dev' parameter tells you the device.
> 
> Anyway, this series is fine, so long as we can agree to the above.

I'm getting very worried that we're spending a lot of time deciding how
users are going to use the system, without getting input from them on
what their use cases are, and are not, and why they've chosen what
they've chosen. Talking with some of the Debian people about why they're
using script + extlinux, and Armbian people about what they're doing
(which is just scripts?) should be the first priority before we decide
how booting things is going to work, outside of the EFI cases (which to
be clear, are having their interests / needs represented already I
believe).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 0/4] cmd: bootefi: refactor the code for bootmgr

[Simon Glass]

Hi AKASHI,

On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> This patch set is motivated by the discussion[1] regarding
> CONFIG_BOOTEFI_BOOTMGR option.
>
> # It has been partially merged in -next branch. So this version (v3)
> # contains only the remaining commits.
>
> At the end, bootefi.c will be decomposed into two parts, one for
> providing the command itself and one for implementing helper functions.
> EFI_LOADER will now be available without CONFIG_CMDLINE or specifically
> CONFIG_CMD_BOOTEFI if invoked via bootmeth/bootstd.

Great!!

>
> Then, EFI_LOADER library side will be further split into two options
> for fine-grain control:
> CONFIG_EFI_BINARY_EXEC: execute UEFI binaries which are to be explicitly
>     loaded by U-Boot's load commands/functions or other methods
>     (like a jtag debugger?)
>     It supports bootmeth_efi as well as "bootefi <addr>|hello"(/"bootm"?).
>
> CONFIG_EFI_BOOTMGR: provide EFI boot manger functionality
>     It supports bootmeth_efi_mgr as well as "bootefi bootmgr".
>
> As such, We will no longer need CONFIG_EFI_BINARY_EXEC if we want to only
> make use of the UEFI boot manger for booting a next stage OS.
>
> Prerequisite
> ============
> This patch set is based on top of Tom's latest "next" branch.
>
> Tests
> =====
> * run UT efi_selftest on sandbox locally
> * run test_efi_bootmgr on sandbox locally
>
> Unfortunately, I could not submit a pull request for CI test.
>
> Changes
> =======
> v3 (Dec 18, 2023)
> * rebased onto Tom's latest next branch
> * remove already-merged commits
>
> v2 (Nov 21, 2023)
> * rebased onto Tom's next branch
> * remove already merged commits
> * revise commit messages
> * add patch #5 which was split from ex-patch#5
> RFC (Oct 26, 2023)
>
> [1] https://lists.denx.de/pipermail/u-boot/2023-October/534598.html
>
> AKASHI Takahiro (4):
>   efi_loader: split unrelated code from efi_bootmgr.c
>   efi_loader: rename BOOTEFI_BOOTMGR to EFI_BOOTMGR
>   net: tftp: remove explicit efi configuration dependency
>   fs: remove explicit efi configuration dependency
>
>  boot/Kconfig                     |   4 +-
>  boot/Makefile                    |   2 +-
>  cmd/Kconfig                      |  10 +-
>  cmd/efidebug.c                   |   4 +-
>  fs/fs.c                          |   7 +-
>  include/efi_loader.h             |  28 +-
>  lib/efi_loader/Kconfig           |  11 +-
>  lib/efi_loader/Makefile          |   2 +-
>  lib/efi_loader/efi_bootmgr.c     | 493 ------------------------------
>  lib/efi_loader/efi_device_path.c |   3 +-
>  lib/efi_loader/efi_helper.c      | 499 ++++++++++++++++++++++++++++++-
>  net/tftp.c                       |  10 +-
>  test/boot/bootflow.c             |   2 +-
>  13 files changed, 544 insertions(+), 531 deletions(-)
>
> --
> 2.34.1
>

Thanks for doing this work.

Regards,
Simon