[PATCH v8 00/25] efi: Improvements to U-Boot running on top of UEFI

[PATCH v8 00/25] efi: Improvements to U-Boot running on top of UEFI

[Simon Glass]

At present U-Boot can be built as an EFI app, but it is really just for
testing, with very few features. Instead, the payload build is used for
booting on top of UEFI, where U-Boot takes over the machine immediately
and supplies its own drivers.

But the app could be made more useful.

This series provides access to EFI block devices and the video console
within U-Boot. It also restructures the app to make it possible to boot
a kernel, although further work is needed to implement this.

This can be thought of as making U-Boot perform a little like 'grub', in
that it runs purely based on UEFI-provided services.

Of course a lot more work is required, but this is a start.

Note: It would be very useful to include qemu tests of the app and stub
in CI.

This is available at u-boot-dm/efi-working

Changes in v8:
- Add new patch to make unicode printf available to the app
- Finish off the comment in add_entry_addr()
- Tidy up the comment as well
- Use the unicode print functions instead of rolling out own

Changes in v7:
- Rebase on -master instead of -next

Changes in v6:
- Add comment for dm_scan_other()
- Add comment for free_memory()
- Drop setup_disks() as U-Boot does not support it
- Fix 'have have' typo
- Fix 'stuff' typo in comment
- Fix Alo typo in commit message
- Fix comment style in setup_info_table()
- Fix typo in function comment
- Update comment format for devpath_is_partition()
- Update to 'This function' in comment
- Use 'U-Boot' instead of 'ARP' typo

Changes in v5:
- Add new patch to avoid setting up global_data again with EFI
- Add new patch to avoid using the 64-bit link script for the EFI app
- Add new patch to build the 64-bit app properly
- Add new patch to round out the link script for 64-bit EFI
- Add new patch to set the correct link flags for the 64-bit EFI app
- Add new patch to tweak the code used for the 64-bit EFI app
- Add various patches to fix up the 64-bit app so that it boots
- Fix grammar in commit message

Changes in v3:
- Add new patch to show the system-table revision
- Drop comments that confuse sphinx
- Move device_path path change to its own patch

Changes in v2:
- Add a better boot command too
- Add a sentence about what the patch does
- Add new patch to support the efi command in the app
- Don't export efi_bind_block()
- Fix 'as' typo
- Only bind devices for media devices, not for partitions
- Show devices that are processed
- Store device path in struct efi_media_plat
- Update documentation
- Use log_info() instead of printf()

Simon Glass (25):
  efi: Make unicode printf available to the app
  efi: Locate all block devices in the app
  efi: serial: Support arrow keys
  x86: Allow booting a kernel from the EFI app
  x86: Don't process the kernel command line unless enabled
  x86: efi: Add room for the binman definition in the dtb
  efi: Drop device_path from struct efi_priv
  efi: Add comments to struct efi_priv
  efi: Fix ll_boot_init() operation with the app
  efi: Add a few comments to the stub
  efi: Share struct efi_priv between the app and stub code
  efi: Move exit_boot_services into a function
  efi: Check for failure when initing the app
  efi: Mention that efi_info_get() is only used in the stub
  efi: Show when allocated pages are used
  efi: Allow easy selection of serial-only operation
  x86: efi: Update efi_get_next_mem_desc() to avoid needing a map
  efi: Support the efi command in the app
  x86: efi: Show the system-table revision
  x86: efi: Don't set up global_data again with EFI
  x86: efi: Tweak the code used for the 64-bit EFI app
  x86: efi: Round out the link script for 64-bit EFI
  x86: efi: Don't use the 64-bit link script for the EFI app
  x86: efi: Set the correct link flags for the 64-bit EFI app
  efi: Build the 64-bit app properly

 Makefile                           |   4 +-
 arch/x86/config.mk                 |  15 +-
 arch/x86/cpu/config.mk             |   2 +-
 arch/x86/cpu/efi/payload.c         |  17 +-
 arch/x86/cpu/x86_64/cpu.c          |   5 +
 arch/x86/dts/Makefile              |   2 +-
 arch/x86/lib/Makefile              |   5 +-
 arch/x86/lib/bootm.c               |  11 +-
 arch/x86/lib/elf_x86_64_efi.lds    |   5 +-
 arch/x86/lib/relocate.c            |   2 +
 arch/x86/lib/zimage.c              |  13 +-
 cmd/Makefile                       |   2 +-
 cmd/efi.c                          |  78 +++++----
 common/board_r.c                   |   5 +-
 doc/develop/uefi/u-boot_on_efi.rst |   6 +-
 drivers/serial/serial_efi.c        |  11 +-
 include/configs/efi-x86_app.h      |  25 +++
 include/efi.h                      | 106 +++++++++++-
 include/efi_api.h                  |  15 ++
 include/init.h                     |   7 +-
 lib/Kconfig                        |   2 +-
 lib/efi/efi.c                      |  97 +++++++++++
 lib/efi/efi_app.c                  | 254 +++++++++++++++++++++++++++--
 lib/efi/efi_stub.c                 | 102 +++++-------
 lib/vsprintf.c                     |   9 +-
 25 files changed, 648 insertions(+), 152 deletions(-)

-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 01/25] efi: Make unicode printf available to the app

[Simon Glass]

This is needed to show unicode strings. Enable this code in the app.

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

Changes in v8:
- Add new patch to make unicode printf available to the app

 lib/Kconfig    | 2 +-
 lib/vsprintf.c | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/Kconfig b/lib/Kconfig
index 807a4c6ade0..ae2b9b60966 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -42,7 +42,7 @@ config CC_OPTIMIZE_LIBS_FOR_SPEED
 
 config CHARSET
 	bool
-	default y if UT_UNICODE || EFI_LOADER || UFS
+	default y if UT_UNICODE || EFI_LOADER || UFS || EFI_APP
 	help
 	  Enables support for various conversions between different
 	  character sets, such as between unicode representations and
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e634bd70b66..de9f236b908 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -276,9 +276,8 @@ static char *string(char *buf, char *end, char *s, int field_width,
 }
 
 /* U-Boot uses UTF-16 strings in the EFI context only. */
-#if CONFIG_IS_ENABLED(EFI_LOADER) && !defined(API_BUILD)
-static char *string16(char *buf, char *end, u16 *s, int field_width,
-		int precision, int flags)
+static __maybe_unused char *string16(char *buf, char *end, u16 *s,
+				     int field_width, int precision, int flags)
 {
 	const u16 *str = s ? s : L"<NULL>";
 	ssize_t i, len = utf16_strnlen(str, precision);
@@ -317,7 +316,6 @@ static char *device_path_string(char *buf, char *end, void *dp, int field_width,
 	return buf;
 }
 #endif
-#endif
 
 static char *mac_address_string(char *buf, char *end, u8 *addr, int field_width,
 				int precision, int flags)
@@ -616,7 +614,8 @@ repeat:
 
 		case 's':
 /* U-Boot uses UTF-16 strings in the EFI context only. */
-#if CONFIG_IS_ENABLED(EFI_LOADER) && !defined(API_BUILD)
+#if (CONFIG_IS_ENABLED(EFI_LOADER) || CONFIG_IS_ENABLED(EFI_APP)) && \
+	!defined(API_BUILD)
 			if (qualifier == 'l') {
 				str = string16(str, end, va_arg(args, u16 *),
 					       field_width, precision, flags);
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 02/25] efi: Locate all block devices in the app

[Simon Glass]

When starting the app, locate all block devices and make them available
to U-Boot. This allows listing partitions and accessing files in
filesystems.

EFI also has the concept of 'disks', meaning boot media. For now, this
is not obviously useful in U-Boot, but add code to at least locate these.
This can be expanded later as needed.

We cannot use printf() in the early stub or app since it is not compiled
in

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

Changes in v8:
- Use the unicode print functions instead of rolling out own

Changes in v6:
- Add comment for dm_scan_other()
- Add comment for free_memory()
- Drop setup_disks() as U-Boot does not support it
- Fix 'have have' typo
- Update comment format for devpath_is_partition()

Changes in v2:
- Don't export efi_bind_block()
- Only bind devices for media devices, not for partitions
- Show devices that are processed
- Store device path in struct efi_media_plat
- Update documentation

 doc/develop/uefi/u-boot_on_efi.rst |   4 +-
 include/efi.h                      |   6 +-
 include/efi_api.h                  |  15 +++
 lib/efi/efi_app.c                  | 197 +++++++++++++++++++++++++++++
 4 files changed, 217 insertions(+), 5 deletions(-)

diff --git a/doc/develop/uefi/u-boot_on_efi.rst b/doc/develop/uefi/u-boot_on_efi.rst
index 5f2f850f071..8f81b799072 100644
--- a/doc/develop/uefi/u-boot_on_efi.rst
+++ b/doc/develop/uefi/u-boot_on_efi.rst
@@ -265,9 +265,7 @@ This work could be extended in a number of ways:
 
 - Figure out how to solve the interrupt problem
 
-- Add more drivers to the application side (e.g. block devices, USB,
-  environment access). This would mostly be an academic exercise as a strong
-  use case is not readily apparent, but it might be fun.
+- Add more drivers to the application side (e.g.USB, environment access).
 
 - Avoid turning off boot services in the stub. Instead allow U-Boot to make
   use of boot services in case it wants to. It is unclear what it might want
diff --git a/include/efi.h b/include/efi.h
index 1432038838a..cd0bdcc717b 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -419,10 +419,12 @@ struct efi_priv {
  *
  * @handle: handle of the controller on which this driver is installed
  * @blkio: block io protocol proxied by this driver
+ * @device_path: EFI path to the device
  */
 struct efi_media_plat {
-	efi_handle_t		handle;
-	struct efi_block_io	*blkio;
+	efi_handle_t handle;
+	struct efi_block_io *blkio;
+	struct efi_device_path *device_path;
 };
 
 /* Base address of the EFI image */
diff --git a/include/efi_api.h b/include/efi_api.h
index 80109f012bc..ec9fa89a935 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -2035,4 +2035,19 @@ struct efi_firmware_management_protocol {
 			const u16 *package_version_name);
 };
 
+#define EFI_DISK_IO_PROTOCOL_GUID	\
+	EFI_GUID(0xce345171, 0xba0b, 0x11d2, 0x8e, 0x4f, \
+		 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
+
+struct efi_disk {
+	u64 revision;
+	efi_status_t (EFIAPI *read_disk)(struct efi_disk *this, u32 media_id,
+					 u64 offset, efi_uintn_t buffer_size,
+					 void *buffer);
+
+	efi_status_t (EFIAPI *write_disk)(struct efi_disk *this, u32 media_id,
+					  u64 offset, efi_uintn_t buffer_size,
+					  void *buffer);
+};
+
 #endif
diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index f61665686c5..852cf3679d6 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -21,6 +21,9 @@
 #include <efi.h>
 #include <efi_api.h>
 #include <sysreset.h>
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+#include <dm/root.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -46,6 +49,49 @@ int efi_info_get(enum efi_entry_t type, void **datap, int *sizep)
 	return -ENOSYS;
 }
 
+/**
+ * efi_bind_block() - bind a new block device to an EFI device
+ *
+ * Binds a new top-level EFI_MEDIA device as well as a child block device so
+ * that the block device can be accessed in U-Boot.
+ *
+ * The device can then be accessed using 'part list efi 0', 'fat ls efi 0:1',
+ * for example, just like any other interface type.
+ *
+ * @handle: handle of the controller on which this driver is installed
+ * @blkio: block io protocol proxied by this driver
+ * @device_path: EFI device path structure for this
+ * @len: Length of @device_path in bytes
+ * @devp: Returns the bound device
+ * @return 0 if OK, -ve on error
+ */
+int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio,
+		   struct efi_device_path *device_path, int len,
+		   struct udevice **devp)
+{
+	struct efi_media_plat plat;
+	struct udevice *dev;
+	char name[18];
+	int ret;
+
+	plat.handle = handle;
+	plat.blkio = blkio;
+	plat.device_path = malloc(device_path->length);
+	if (!plat.device_path)
+		return log_msg_ret("path", -ENOMEM);
+	memcpy(plat.device_path, device_path, device_path->length);
+	ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
+			  &plat, ofnode_null(), &dev);
+	if (ret)
+		return log_msg_ret("bind", ret);
+
+	snprintf(name, sizeof(name), "efi_media_%x", dev_seq(dev));
+	device_set_name(dev, name);
+	*devp = dev;
+
+	return 0;
+}
+
 static efi_status_t setup_memory(struct efi_priv *priv)
 {
 	struct efi_boot_services *boot = priv->boot;
@@ -91,6 +137,14 @@ static efi_status_t setup_memory(struct efi_priv *priv)
 	return 0;
 }
 
+/**
+ * free_memory() - Free memory used by the U-Boot app
+ *
+ * This frees memory allocated in setup_memory(), in preparation for returning
+ * to UEFI. It also zeroes the global_data pointer.
+ *
+ * @priv: Private EFI data
+ */
 static void free_memory(struct efi_priv *priv)
 {
 	struct efi_boot_services *boot = priv->boot;
@@ -105,6 +159,149 @@ static void free_memory(struct efi_priv *priv)
 	global_data_ptr = NULL;
 }
 
+/**
+ * devpath_is_partition() - Figure out if a device path is a partition
+ *
+ * Checks if a device path refers to a partition on some media device. This
+ * works by checking for a valid partition number in a hard-driver media device
+ * as the final component of the device path.
+ *
+ * Return: true if a partition, false if not (e.g. it might be media which
+ *	contains partitions)
+ */
+static bool devpath_is_partition(const struct efi_device_path *path)
+{
+	const struct efi_device_path *p;
+	bool was_part;
+
+	for (p = path; p->type != DEVICE_PATH_TYPE_END;
+	     p = (void *)p + p->length) {
+		was_part = false;
+		if (p->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
+		    p->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
+			struct efi_device_path_hard_drive_path *hd =
+				(void *)path;
+
+			if (hd->partition_number)
+				was_part = true;
+		}
+	}
+
+	return was_part;
+}
+
+/**
+ * setup_block() - Find all block devices and setup EFI devices for them
+ *
+ * Partitions are ignored, since U-Boot has partition handling. Errors with
+ * particular devices produce a warning but execution continues to try to
+ * find others.
+ *
+ * Return: 0 if found, -ENOSYS if there is no boot-services table, -ENOTSUPP
+ *	if a required protocol is not supported
+ */
+static int setup_block(void)
+{
+	efi_guid_t efi_blkio_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
+	efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
+	efi_guid_t efi_pathutil_guid = EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID;
+	efi_guid_t efi_pathtext_guid = EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
+	struct efi_boot_services *boot = efi_get_boot();
+	struct efi_device_path_utilities_protocol *util;
+	struct efi_device_path_to_text_protocol *text;
+	struct efi_device_path *path;
+	struct efi_block_io *blkio;
+	efi_uintn_t num_handles;
+	efi_handle_t *handle;
+	int ret, i;
+
+	if (!boot)
+		return log_msg_ret("sys", -ENOSYS);
+
+	/* Find all devices which support the block I/O protocol */
+	ret = boot->locate_handle_buffer(BY_PROTOCOL, &efi_blkio_guid, NULL,
+				  &num_handles, &handle);
+	if (ret)
+		return log_msg_ret("loc", -ENOTSUPP);
+	log_debug("Found %d handles:\n", (int)num_handles);
+
+	/* We need to look up the path size and convert it to text */
+	ret = boot->locate_protocol(&efi_pathutil_guid, NULL, (void **)&util);
+	if (ret)
+		return log_msg_ret("util", -ENOTSUPP);
+	ret = boot->locate_protocol(&efi_pathtext_guid, NULL, (void **)&text);
+	if (ret)
+		return log_msg_ret("text", -ENOTSUPP);
+
+	for (i = 0; i < num_handles; i++) {
+		struct udevice *dev;
+		const u16 *name;
+		bool is_part;
+		int len;
+
+		ret = boot->handle_protocol(handle[i], &efi_devpath_guid,
+					    (void **)&path);
+		if (ret) {
+			log_warning("- devpath %d failed (ret=%d)\n", i, ret);
+			continue;
+		}
+
+		ret = boot->handle_protocol(handle[i], &efi_blkio_guid,
+					    (void **)&blkio);
+		if (ret) {
+			log_warning("- blkio %d failed (ret=%d)\n", i, ret);
+			continue;
+		}
+
+		name = text->convert_device_path_to_text(path, true, false);
+		is_part = devpath_is_partition(path);
+
+		if (!is_part) {
+			len = util->get_device_path_size(path);
+			ret = efi_bind_block(handle[i], blkio, path, len, &dev);
+			if (ret) {
+				log_warning("- blkio bind %d failed (ret=%d)\n",
+					    i, ret);
+				continue;
+			}
+		} else {
+			dev = NULL;
+		}
+
+		/*
+		 * Show the device name if we created one. Otherwise indicate
+		 * that it is a partition.
+		 */
+		printf("%2d: %-12s %ls\n", i, dev ? dev->name : "<partition>",
+		       name);
+	}
+	boot->free_pool(handle);
+
+	return 0;
+}
+
+/**
+ * dm_scan_other() - Scan for UEFI devices that should be available to U-Boot
+ *
+ * This sets up block devices within U-Boot for those found in UEFI. With this,
+ * U-Boot can access those devices
+ *
+ * @pre_reloc_only: true to only bind pre-relocation devices (ignored)
+ * Returns: 0 on success, -ve on error
+ */
+int dm_scan_other(bool pre_reloc_only)
+{
+	if (gd->flags & GD_FLG_RELOC) {
+		int ret;
+
+		ret = setup_block();
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * efi_main() - Start an EFI image
  *
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 03/25] efi: serial: Support arrow keys

[Simon Glass]

At present only the backspace key is supported in U-Boot, when running as
an EFI app. Add support for arrows, home and end as well, to make the CLI
more friendly.

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

(no changes since v1)

 drivers/serial/serial_efi.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/serial_efi.c b/drivers/serial/serial_efi.c
index 33ddbd6080c..0067576389d 100644
--- a/drivers/serial/serial_efi.c
+++ b/drivers/serial/serial_efi.c
@@ -24,6 +24,9 @@ struct serial_efi_priv {
 	bool have_key;
 };
 
+/* Convert a lower-case character to its ctrl-char equivalent */
+#define CTL_CH(c)		((c) - 'a' + 1)
+
 int serial_efi_setbrg(struct udevice *dev, int baudrate)
 {
 	return 0;
@@ -49,6 +52,7 @@ static int serial_efi_get_key(struct serial_efi_priv *priv)
 static int serial_efi_getc(struct udevice *dev)
 {
 	struct serial_efi_priv *priv = dev_get_priv(dev);
+	char conv_scan[10] = {0, 'p', 'n', 'f', 'b', 'a', 'e', 0, 8};
 	int ret, ch;
 
 	ret = serial_efi_get_key(priv);
@@ -63,8 +67,11 @@ static int serial_efi_getc(struct udevice *dev)
 	 * key scan code of 8. Handle this so that backspace works correctly
 	 * in the U-Boot command line.
 	 */
-	if (!ch && priv->key.scan_code == 8)
-		ch = 8;
+	if (!ch && priv->key.scan_code < sizeof(conv_scan)) {
+		ch = conv_scan[priv->key.scan_code];
+		if (ch >= 'a')
+			ch -= 'a' - 1;
+	}
 	debug(" [%x %x %x] ", ch, priv->key.unicode_char, priv->key.scan_code);
 
 	return ch;
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 04/25] x86: Allow booting a kernel from the EFI app

[Simon Glass]

At present this is disabled, but it should work so long as the kernel does
not need EFI services. Enable it and add a note about remaining work.

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

(no changes since v2)

Changes in v2:
- Update documentation

 arch/x86/lib/bootm.c               | 11 +++++++----
 doc/develop/uefi/u-boot_on_efi.rst |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
index 667e5e689e3..57cba5c65d3 100644
--- a/arch/x86/lib/bootm.c
+++ b/arch/x86/lib/bootm.c
@@ -179,10 +179,14 @@ int boot_linux_kernel(ulong setup_base, ulong load_address, bool image_64bit)
 		* U-Boot is setting them up that way for itself in
 		* arch/i386/cpu/cpu.c.
 		*
-		* Note that we cannot currently boot a kernel while running as
-		* an EFI application. Please use the payload option for that.
+		* Note: this is incomplete for EFI kernels!
+		*
+		* This can boot a kernel while running as an EFI application,
+		* but if the kernel requires EFI support then that support needs
+		* to be enabled first (see EFI_LOADER). Also the EFI information
+		* must enabled with setup_efi_info(). See setup_zimage() for
+		* how this is done with the stub.
 		*/
-#ifndef CONFIG_EFI_APP
 		__asm__ __volatile__ (
 		"movl $0, %%ebp\n"
 		"cli\n"
@@ -191,7 +195,6 @@ int boot_linux_kernel(ulong setup_base, ulong load_address, bool image_64bit)
 		[boot_params] "S"(setup_base),
 		"b"(0), "D"(0)
 		);
-#endif
 	}
 
 	/* We can't get to here */
diff --git a/doc/develop/uefi/u-boot_on_efi.rst b/doc/develop/uefi/u-boot_on_efi.rst
index 8f81b799072..acad6397e81 100644
--- a/doc/develop/uefi/u-boot_on_efi.rst
+++ b/doc/develop/uefi/u-boot_on_efi.rst
@@ -269,7 +269,7 @@ This work could be extended in a number of ways:
 
 - Avoid turning off boot services in the stub. Instead allow U-Boot to make
   use of boot services in case it wants to. It is unclear what it might want
-  though.
+  though. It is better to use the app.
 
 Where is the code?
 ------------------
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 05/25] x86: Don't process the kernel command line unless enabled

[Simon Glass]

If the 'bootm' command is not enabled then this code is not available and
this causes a link error. Fix it.

Note that for the EFI app, there is no indication of missing code. It just
hangs!

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

(no changes since v1)

 arch/x86/lib/zimage.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
index 7ce02226ef9..9cc04490307 100644
--- a/arch/x86/lib/zimage.c
+++ b/arch/x86/lib/zimage.c
@@ -365,11 +365,14 @@ int setup_zimage(struct boot_params *setup_base, char *cmd_line, int auto_boot,
 			strcpy(cmd_line, (char *)cmdline_force);
 		else
 			build_command_line(cmd_line, auto_boot);
-		ret = bootm_process_cmdline(cmd_line, max_size, BOOTM_CL_ALL);
-		if (ret) {
-			printf("Cmdline setup failed (max_size=%x, bootproto=%x, err=%d)\n",
-			       max_size, bootproto, ret);
-			return ret;
+		if (IS_ENABLED(CONFIG_CMD_BOOTM)) {
+			ret = bootm_process_cmdline(cmd_line, max_size,
+						    BOOTM_CL_ALL);
+			if (ret) {
+				printf("Cmdline setup failed (max_size=%x, bootproto=%x, err=%d)\n",
+				       max_size, bootproto, ret);
+				return ret;
+			}
 		}
 		printf("Kernel command line: \"");
 		puts(cmd_line);
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 06/25] x86: efi: Add room for the binman definition in the dtb

[Simon Glass]

At present only 4KB of spare space is left in the DTB when building the
EFI app. Increase this to 32KB so there is plenty of space to insert the
binman definition. This cannot be expanded later (as with OF_SEPARATE)
because the ELF image has already been built.

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

(no changes since v1)

 arch/x86/dts/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/dts/Makefile b/arch/x86/dts/Makefile
index be209aaaf8f..5c8c05ec499 100644
--- a/arch/x86/dts/Makefile
+++ b/arch/x86/dts/Makefile
@@ -24,7 +24,7 @@ dtb-y += bayleybay.dtb \
 
 targets += $(dtb-y)
 
-DTC_FLAGS += -R 4 -p 0x1000
+DTC_FLAGS += -R 4 -p $(if $(CONFIG_EFI_APP),0x8000,0x1000)
 
 PHONY += dtbs
 dtbs: $(addprefix $(obj)/, $(dtb-y))
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 07/25] efi: Drop device_path from struct efi_priv

[Simon Glass]

This is not used anywhere drop it.

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

(no changes since v3)

Changes in v3:
- Move device_path path change to its own patch

 include/efi.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/efi.h b/include/efi.h
index cd0bdcc717b..0cd4b46600e 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -402,7 +402,6 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
 
 struct efi_priv {
 	efi_handle_t parent_image;
-	struct efi_device_path *device_path;
 	struct efi_system_table *sys_table;
 	struct efi_boot_services *boot;
 	struct efi_runtime_services *run;
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 08/25] efi: Add comments to struct efi_priv

[Simon Glass]

This structure is uncommented. Fix it.

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

(no changes since v3)

Changes in v3:
- Drop comments that confuse sphinx
- Move device_path path change to its own patch

 include/efi.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/efi.h b/include/efi.h
index 0cd4b46600e..57ca2f424ab 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -400,14 +400,37 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
 	return (struct efi_mem_desc *)((ulong)desc + map->desc_size);
 }
 
+/**
+ * struct efi_priv - Information about the environment provided by EFI
+ *
+ * @parent_image: image passed into the EFI app or stub
+ * @sys_table: Pointer to system table
+ * @boot: Pointer to boot-services table
+ * @run: Pointer to runtime-services table
+ *
+ * @use_pool_for_malloc: true if all allocation should go through the EFI 'pool'
+ *	methods allocate_pool() and free_pool(); false to use 'pages' methods
+ *	allocate_pages() and free_pages()
+ * @ram_base: Base address of RAM (size CONFIG_EFI_RAM_SIZE)
+ * @image_data_type: Type of the loaded image (e.g. EFI_LOADER_CODE)
+ *
+ * @info: Header of the info list, holding info collected by the stub and passed
+ *	to U-Boot
+ * @info_size: Size of the info list, in bytes from @info
+ * @next_hdr: Pointer to where to put the next header when adding to the list
+ */
 struct efi_priv {
 	efi_handle_t parent_image;
 	struct efi_system_table *sys_table;
 	struct efi_boot_services *boot;
 	struct efi_runtime_services *run;
+
+	/* app: */
 	bool use_pool_for_malloc;
 	unsigned long ram_base;
 	unsigned int image_data_type;
+
+	/* stub: */
 	struct efi_info_hdr *info;
 	unsigned int info_size;
 	void *next_hdr;
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 09/25] efi: Fix ll_boot_init() operation with the app

[Simon Glass]

This should return false when the EFI app is running, since UEFI has done
the required low-level init. Fix it.

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

Changes in v8:
- Tidy up the comment as well

 include/init.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/init.h b/include/init.h
index f2cd46dead0..dcd682c1bf6 100644
--- a/include/init.h
+++ b/include/init.h
@@ -14,8 +14,11 @@
 
 #include <linux/types.h>
 
-/* Avoid using CONFIG_EFI_STUB directly as we may boot from other loaders */
-#ifdef CONFIG_EFI_STUB
+/*
+ * In case of the EFI app the UEFI firmware provides the low-level
+ * initialisation.
+ */
+#ifdef CONFIG_EFI
 #define ll_boot_init()	false
 #else
 #include <asm/global_data.h>
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 10/25] efi: Add a few comments to the stub

[Simon Glass]

Comment some functions that need more information.

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

Changes in v8:
- Finish off the comment in add_entry_addr()

Changes in v6:
- Fix comment style in setup_info_table()

 lib/efi/efi_stub.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c
index b3393e47fae..31f1e1a72a1 100644
--- a/lib/efi/efi_stub.c
+++ b/lib/efi/efi_stub.c
@@ -225,6 +225,22 @@ static int get_codeseg32(void)
 	return cs32;
 }
 
+/**
+ * setup_info_table() - sets up a table containing information from EFI
+ *
+ * We must call exit_boot_services() before jumping out of the stub into U-Boot
+ * proper, so that U-Boot has full control of peripherals, memory, etc.
+ *
+ * Once we do this, we cannot call any boot-services functions so we must find
+ * out everything we need to before doing that.
+ *
+ * Set up a struct efi_info_hdr table which can hold various records (e.g.
+ * struct efi_entry_memmap) with information obtained from EFI.
+ *
+ * @priv: Pointer to our private information which contains the list
+ * @size: Size of the table to allocate
+ * Return: 0 if OK, non-zero on error
+ */
 static int setup_info_table(struct efi_priv *priv, int size)
 {
 	struct efi_info_hdr *info;
@@ -248,6 +264,19 @@ static int setup_info_table(struct efi_priv *priv, int size)
 	return 0;
 }
 
+/**
+ * add_entry_addr() - Add a new entry to the efi_info list
+ *
+ * This adds an entry, consisting of a tag and two lots of data. This avoids the
+ * caller having to coalesce the data first
+ *
+ * @priv: Pointer to our private information which contains the list
+ * @type: Type of the entry to add
+ * @ptr1: Pointer to first data block to add
+ * @size1: Size of first data block in bytes (can be 0)
+ * @ptr2: Pointer to second data block to add
+ * @size2: Size of second data block in bytes (can be 0)
+ */
 static void add_entry_addr(struct efi_priv *priv, enum efi_entry_t type,
 			   void *ptr1, int size1, void *ptr2, int size2)
 {
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 11/25] efi: Share struct efi_priv between the app and stub code

[Simon Glass]

At present each of these has its own static variable and helper functions.
Move them into a shared file.

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

(no changes since v1)

 include/efi.h      | 21 +++++++++++++++++++++
 lib/efi/efi.c      | 29 +++++++++++++++++++++++++++++
 lib/efi/efi_app.c  | 21 ++-------------------
 lib/efi/efi_stub.c |  7 ++++---
 4 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/include/efi.h b/include/efi.h
index 57ca2f424ab..d4785478585 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -474,6 +474,27 @@ extern char _binary_u_boot_bin_start[], _binary_u_boot_bin_end[];
 				EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS | \
 				EFI_VARIABLE_APPEND_WRITE)
 
+/**
+ * efi_get_priv() - Get access to the EFI-private information
+ *
+ * This struct it used by both the stub and the app to record things about the
+ * EFI environment. It is not available in U-Boot proper after the stub has
+ * jumped there. Use efi_info_get() to obtain info in that case.
+ *
+ * @return pointer to private info
+ */
+struct efi_priv *efi_get_priv(void);
+
+/**
+ * efi_set_priv() - Set up a pointer to the EFI-private information
+ *
+ * This is called in the stub and app to record the location of this
+ * information.
+ *
+ * @priv: New location of private data
+ */
+void efi_set_priv(struct efi_priv *priv);
+
 /**
  * efi_get_sys_table() - Get access to the main EFI system table
  *
diff --git a/lib/efi/efi.c b/lib/efi/efi.c
index 69e52e45748..cd6bf47b180 100644
--- a/lib/efi/efi.c
+++ b/lib/efi/efi.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
+ * Functions shared by the app and stub
+ *
  * Copyright (c) 2015 Google, Inc
  *
  * EFI information obtained here:
@@ -17,6 +19,33 @@
 #include <efi.h>
 #include <efi_api.h>
 
+static struct efi_priv *global_priv;
+
+struct efi_priv *efi_get_priv(void)
+{
+	return global_priv;
+}
+
+void efi_set_priv(struct efi_priv *priv)
+{
+	global_priv = priv;
+}
+
+struct efi_system_table *efi_get_sys_table(void)
+{
+	return global_priv->sys_table;
+}
+
+struct efi_boot_services *efi_get_boot(void)
+{
+	return global_priv->boot;
+}
+
+unsigned long efi_get_ram_base(void)
+{
+	return global_priv->ram_base;
+}
+
 /*
  * Global declaration of gd.
  *
diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index 852cf3679d6..2f1feda1b1e 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -27,23 +27,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static struct efi_priv *global_priv;
-
-struct efi_system_table *efi_get_sys_table(void)
-{
-	return global_priv->sys_table;
-}
-
-struct efi_boot_services *efi_get_boot(void)
-{
-	return global_priv->boot;
-}
-
-unsigned long efi_get_ram_base(void)
-{
-	return global_priv->ram_base;
-}
-
 int efi_info_get(enum efi_entry_t type, void **datap, int *sizep)
 {
 	return -ENOSYS;
@@ -318,7 +301,7 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 	/* Set up access to EFI data structures */
 	efi_init(priv, "App", image, sys_table);
 
-	global_priv = priv;
+	efi_set_priv(priv);
 
 	/*
 	 * Set up the EFI debug UART so that printf() works. This is
@@ -344,7 +327,7 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 
 static void efi_exit(void)
 {
-	struct efi_priv *priv = global_priv;
+	struct efi_priv *priv = efi_get_priv();
 
 	free_memory(priv);
 	printf("U-Boot EFI exiting\n");
diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c
index 31f1e1a72a1..c89ae7c9072 100644
--- a/lib/efi/efi_stub.c
+++ b/lib/efi/efi_stub.c
@@ -31,7 +31,6 @@
 #error "This file needs to be ported for use on architectures"
 #endif
 
-static struct efi_priv *global_priv;
 static bool use_uart;
 
 struct __packed desctab_info {
@@ -63,6 +62,8 @@ void _debug_uart_init(void)
 
 void putc(const char ch)
 {
+	struct efi_priv *priv = efi_get_priv();
+
 	if (ch == '\n')
 		putc('\r');
 
@@ -73,7 +74,7 @@ void putc(const char ch)
 			;
 		outb(ch, (ulong)&com_port->thr);
 	} else {
-		efi_putc(global_priv, ch);
+		efi_putc(priv, ch);
 	}
 }
 
@@ -320,7 +321,7 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 		puts(" efi_init() failed\n");
 		return ret;
 	}
-	global_priv = priv;
+	efi_set_priv(priv);
 
 	cs32 = get_codeseg32();
 	if (cs32 < 0)
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 12/25] efi: Move exit_boot_services into a function

[Simon Glass]

At present this code is inline in the app and stub. But they do the same
thing. The difference is that the stub does it immediately and the app
doesn't want to do it until the end (when it boots a kernel) or not at
all, if returning to UEFI.

Move it into a function so it can be called as needed.

Also store the memory map so that it can be accessed within the app if
needed.

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

(no changes since v6)

Changes in v6:
- Fix typo in function comment

Changes in v2:
- Add a sentence about what the patch does

 include/efi.h      | 32 ++++++++++++++++++++++
 lib/efi/efi.c      | 68 ++++++++++++++++++++++++++++++++++++++++++++++
 lib/efi/efi_app.c  |  3 ++
 lib/efi/efi_stub.c | 66 ++++++++------------------------------------
 4 files changed, 114 insertions(+), 55 deletions(-)

diff --git a/include/efi.h b/include/efi.h
index d4785478585..2a84223d235 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -407,6 +407,12 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
  * @sys_table: Pointer to system table
  * @boot: Pointer to boot-services table
  * @run: Pointer to runtime-services table
+ * @memmap_key: Key returned from get_memory_map()
+ * @memmap_desc: List of memory-map records
+ * @memmap_alloc: Amount of memory allocated for memory map list
+ * @memmap_size Size of memory-map list in bytes
+ * @memmap_desc_size: Size of an individual memory-map record, in bytes
+ * @memmap_version: Memory-map version
  *
  * @use_pool_for_malloc: true if all allocation should go through the EFI 'pool'
  *	methods allocate_pool() and free_pool(); false to use 'pages' methods
@@ -424,6 +430,12 @@ struct efi_priv {
 	struct efi_system_table *sys_table;
 	struct efi_boot_services *boot;
 	struct efi_runtime_services *run;
+	efi_uintn_t memmap_key;
+	struct efi_mem_desc *memmap_desc;
+	efi_uintn_t memmap_alloc;
+	efi_uintn_t memmap_size;
+	efi_uintn_t memmap_desc_size;
+	u32 memmap_version;
 
 	/* app: */
 	bool use_pool_for_malloc;
@@ -574,4 +586,24 @@ void efi_putc(struct efi_priv *priv, const char ch);
  */
 int efi_info_get(enum efi_entry_t type, void **datap, int *sizep);
 
+/**
+ * efi_store_memory_map() - Collect the memory-map info from EFI
+ *
+ * Collect the memory info and store it for later use, e.g. in calling
+ * exit_boot_services()
+ *
+ * @priv:	Pointer to private EFI structure
+ * @return 0 if OK, non-zero on error
+ */
+int efi_store_memory_map(struct efi_priv *priv);
+
+/**
+ * efi_call_exit_boot_services() - Handle the exit-boot-service procedure
+ *
+ * Tell EFI we don't want their boot services anymore
+ *
+ * Return: 0 if OK, non-zero on error
+ */
+int efi_call_exit_boot_services(void);
+
 #endif /* _LINUX_EFI_H */
diff --git a/lib/efi/efi.c b/lib/efi/efi.c
index cd6bf47b180..20da88c9151 100644
--- a/lib/efi/efi.c
+++ b/lib/efi/efi.c
@@ -135,3 +135,71 @@ void efi_free(struct efi_priv *priv, void *ptr)
 
 	boot->free_pool(ptr);
 }
+
+int efi_store_memory_map(struct efi_priv *priv)
+{
+	struct efi_boot_services *boot = priv->sys_table->boottime;
+	efi_uintn_t size, desc_size;
+	efi_status_t ret;
+
+	/* Get the memory map so we can switch off EFI */
+	size = 0;
+	ret = boot->get_memory_map(&size, NULL, &priv->memmap_key,
+				   &priv->memmap_desc_size,
+				   &priv->memmap_version);
+	if (ret != EFI_BUFFER_TOO_SMALL) {
+		printhex2(EFI_BITS_PER_LONG);
+		putc(' ');
+		printhex2(ret);
+		puts(" No memory map\n");
+		return ret;
+	}
+	/*
+	 * Since doing a malloc() may change the memory map and also we want to
+	 * be able to read the memory map in efi_call_exit_boot_services()
+	 * below, after more changes have happened
+	 */
+	priv->memmap_alloc = size + 1024;
+	priv->memmap_size = priv->memmap_alloc;
+	priv->memmap_desc = efi_malloc(priv, size, &ret);
+	if (!priv->memmap_desc) {
+		printhex2(ret);
+		puts(" No memory for memory descriptor\n");
+		return ret;
+	}
+
+	ret = boot->get_memory_map(&priv->memmap_size, priv->memmap_desc,
+				   &priv->memmap_key, &desc_size,
+				   &priv->memmap_version);
+	if (ret) {
+		printhex2(ret);
+		puts(" Can't get memory map\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+int efi_call_exit_boot_services(void)
+{
+	struct efi_priv *priv = efi_get_priv();
+	const struct efi_boot_services *boot = priv->boot;
+	efi_uintn_t size;
+	u32 version;
+	efi_status_t ret;
+
+	size = priv->memmap_alloc;
+	ret = boot->get_memory_map(&size, priv->memmap_desc,
+				   &priv->memmap_key,
+				   &priv->memmap_desc_size, &version);
+	if (ret) {
+		printhex2(ret);
+		puts(" Can't get memory map\n");
+		return ret;
+	}
+	ret = boot->exit_boot_services(priv->parent_image, priv->memmap_key);
+	if (ret)
+		return ret;
+
+	return 0;
+}
diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index 2f1feda1b1e..8bd710d2e95 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -315,6 +315,9 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 		printf("Failed to set up memory: ret=%lx\n", ret);
 		return ret;
 	}
+	ret = efi_store_memory_map(priv);
+	if (ret)
+		return ret;
 
 	printf("starting\n");
 
diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c
index c89ae7c9072..646cde3214c 100644
--- a/lib/efi/efi_stub.c
+++ b/lib/efi/efi_stub.c
@@ -304,15 +304,12 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 {
 	struct efi_priv local_priv, *priv = &local_priv;
 	struct efi_boot_services *boot = sys_table->boottime;
-	struct efi_mem_desc *desc;
 	struct efi_entry_memmap map;
 	struct efi_gop *gop;
 	struct efi_entry_gopmode mode;
 	struct efi_entry_systable table;
 	efi_guid_t efi_gop_guid = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
-	efi_uintn_t key, desc_size, size;
 	efi_status_t ret;
-	u32 version;
 	int cs32;
 
 	ret = efi_init(priv, "Payload", image, sys_table);
@@ -327,24 +324,11 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 	if (cs32 < 0)
 		return EFI_UNSUPPORTED;
 
-	/* Get the memory map so we can switch off EFI */
-	size = 0;
-	ret = boot->get_memory_map(&size, NULL, &key, &desc_size, &version);
-	if (ret != EFI_BUFFER_TOO_SMALL) {
-		printhex2(EFI_BITS_PER_LONG);
-		putc(' ');
-		printhex2(ret);
-		puts(" No memory map\n");
-		return ret;
-	}
-	size += 1024;	/* Since doing a malloc() may change the memory map! */
-	desc = efi_malloc(priv, size, &ret);
-	if (!desc) {
-		printhex2(ret);
-		puts(" No memory for memory descriptor\n");
+	ret = efi_store_memory_map(priv);
+	if (ret)
 		return ret;
-	}
-	ret = setup_info_table(priv, size + 128);
+
+	ret = setup_info_table(priv, priv->memmap_size + 128);
 	if (ret)
 		return ret;
 
@@ -360,48 +344,20 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 			       sizeof(struct efi_gop_mode_info));
 	}
 
-	ret = boot->get_memory_map(&size, desc, &key, &desc_size, &version);
-	if (ret) {
-		printhex2(ret);
-		puts(" Can't get memory map\n");
-		return ret;
-	}
-
 	table.sys_table = (ulong)sys_table;
 	add_entry_addr(priv, EFIET_SYS_TABLE, &table, sizeof(table), NULL, 0);
 
-	ret = boot->exit_boot_services(image, key);
-	if (ret) {
-		/*
-		 * Unfortunately it happens that we cannot exit boot services
-		 * the first time. But the second time it work. I don't know
-		 * why but this seems to be a repeatable problem. To get
-		 * around it, just try again.
-		 */
-		printhex2(ret);
-		puts(" Can't exit boot services\n");
-		size = sizeof(desc);
-		ret = boot->get_memory_map(&size, desc, &key, &desc_size,
-					   &version);
-		if (ret) {
-			printhex2(ret);
-			puts(" Can't get memory map\n");
-			return ret;
-		}
-		ret = boot->exit_boot_services(image, key);
-		if (ret) {
-			printhex2(ret);
-			puts(" Can't exit boot services 2\n");
-			return ret;
-		}
-	}
+	ret = efi_call_exit_boot_services();
+	if (ret)
+		return ret;
 
 	/* The EFI UART won't work now, switch to a debug one */
 	use_uart = true;
 
-	map.version = version;
-	map.desc_size = desc_size;
-	add_entry_addr(priv, EFIET_MEMORY_MAP, &map, sizeof(map), desc, size);
+	map.version = priv->memmap_version;
+	map.desc_size = priv->memmap_desc_size;
+	add_entry_addr(priv, EFIET_MEMORY_MAP, &map, sizeof(map),
+		       priv->memmap_desc, priv->memmap_size);
 	add_entry_addr(priv, EFIET_END, NULL, 0, 0, 0);
 
 	memcpy((void *)CONFIG_SYS_TEXT_BASE, _binary_u_boot_bin_start,
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 13/25] efi: Check for failure when initing the app

[Simon Glass]

The stub checks for failure with efi_init(). Add this for the app as well.
It is unlikely that anything can be done, but we may as well stop.

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

(no changes since v6)

Changes in v6:
- Use 'U-Boot' instead of 'ARP' typo

 lib/efi/efi_app.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index 8bd710d2e95..30af414069d 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -299,8 +299,11 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
 	efi_status_t ret;
 
 	/* Set up access to EFI data structures */
-	efi_init(priv, "App", image, sys_table);
-
+	ret = efi_init(priv, "App", image, sys_table);
+	if (ret) {
+		printf("Failed to set up U-Boot: err=%lx\n", ret);
+		return ret;
+	}
 	efi_set_priv(priv);
 
 	/*
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 14/25] efi: Mention that efi_info_get() is only used in the stub

[Simon Glass]

This provides access to EFI tables after U-Boot has exited boot services.
It is not needed in the app since boot services remain alive and we can
just call them whenever needed.

Add a comment to explain this.

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

(no changes since v6)

Changes in v6:
- Fix 'stuff' typo in comment
- Update to 'This function' in comment

Changes in v2:
- Fix 'as' typo

 include/efi.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/efi.h b/include/efi.h
index 2a84223d235..dbf4ffaaf02 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -578,6 +578,10 @@ void efi_putc(struct efi_priv *priv, const char ch);
 /**
  * efi_info_get() - get an entry from an EFI table
  *
+ * This function is called from U-Boot proper to read information set up by the
+ * EFI stub. It can only be used when running from the EFI stub, not when U-Boot
+ * is running as an app.
+ *
  * @type:	Entry type to search for
  * @datap:	Returns pointer to entry data
  * @sizep:	Returns pointer to entry size
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 15/25] efi: Show when allocated pages are used

[Simon Glass]

Add a message here so that both paths of memory allocation are reported.

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

(no changes since v2)

Changes in v2:
- Use log_info() instead of printf()

 lib/efi/efi_app.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index 30af414069d..f5eab0823cf 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -106,13 +106,14 @@ static efi_status_t setup_memory(struct efi_priv *priv)
 	ret = boot->allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
 				   priv->image_data_type, pages, &addr);
 	if (ret) {
-		printf("(using pool %lx) ", ret);
+		log_info("(using pool %lx) ", ret);
 		priv->ram_base = (ulong)efi_malloc(priv, CONFIG_EFI_RAM_SIZE,
 						   &ret);
 		if (!priv->ram_base)
 			return ret;
 		priv->use_pool_for_malloc = true;
 	} else {
+		log_info("(using allocated RAM address %lx) ", (ulong)addr);
 		priv->ram_base = addr;
 	}
 	gd->ram_size = pages << 12;
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 16/25] efi: Allow easy selection of serial-only operation

[Simon Glass]

Add info about how to select vidconsole or serial.

Also set up a demo boot command.

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

(no changes since v2)

Changes in v2:
- Add a better boot command too

 include/configs/efi-x86_app.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/configs/efi-x86_app.h b/include/configs/efi-x86_app.h
index 6061a6db0a4..33afb7ca0f9 100644
--- a/include/configs/efi-x86_app.h
+++ b/include/configs/efi-x86_app.h
@@ -10,8 +10,33 @@
 
 #undef CONFIG_TPM_TIS_BASE_ADDRESS
 
+/*
+ * Select the output device: Put an 'x' prefix before one of these to disable it
+ */
+
+/*
+ * Video output - can normally continue after exit_boot_services has been
+ * called, since output to the display does not require EFI services at that
+ * point. U-Boot sets up the console memory and does its own drawing.
+ */
 #define CONFIG_STD_DEVICES_SETTINGS	"stdin=serial\0" \
 					"stdout=vidconsole\0" \
 					"stderr=vidconsole\0"
 
+/*
+ * Serial output with no console. Run qemu with:
+ *
+ *    -display none -serial mon:stdio
+ *
+ * This will hang or fail to output on the console after exit_boot_services is
+ * called.
+ */
+#define xCONFIG_STD_DEVICES_SETTINGS	"stdin=serial\0" \
+					"stdout=serial\0" \
+					"stderr=serial\0"
+
+#undef CONFIG_BOOTCOMMAND
+
+#define CONFIG_BOOTCOMMAND "part list efi 0; fatls efi 0:1"
+
 #endif
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 17/25] x86: efi: Update efi_get_next_mem_desc() to avoid needing a map

[Simon Glass]

At present this function requires a pointer to struct efi_entry_memmap
but the only field used in there is the desc_size. We want to be able
to use it from the app, so update it to use desc_size directly.

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

(no changes since v1)

 arch/x86/cpu/efi/payload.c |  8 ++++----
 cmd/efi.c                  | 34 ++++++++++++++++++----------------
 include/efi.h              |  4 ++--
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/arch/x86/cpu/efi/payload.c b/arch/x86/cpu/efi/payload.c
index 3a9f7d72868..d2aa889a2b9 100644
--- a/arch/x86/cpu/efi/payload.c
+++ b/arch/x86/cpu/efi/payload.c
@@ -50,7 +50,7 @@ ulong board_get_usable_ram_top(ulong total_size)
 
 	end = (struct efi_mem_desc *)((ulong)map + size);
 	desc = map->desc;
-	for (; desc < end; desc = efi_get_next_mem_desc(map, desc)) {
+	for (; desc < end; desc = efi_get_next_mem_desc(desc, map->desc_size)) {
 		if (desc->type != EFI_CONVENTIONAL_MEMORY ||
 		    desc->physical_start >= 1ULL << 32)
 			continue;
@@ -88,7 +88,7 @@ int dram_init(void)
 	end = (struct efi_mem_desc *)((ulong)map + size);
 	gd->ram_size = 0;
 	desc = map->desc;
-	for (; desc < end; desc = efi_get_next_mem_desc(map, desc)) {
+	for (; desc < end; desc = efi_get_next_mem_desc(desc, map->desc_size)) {
 		if (desc->type < EFI_MMAP_IO)
 			gd->ram_size += desc->num_pages << EFI_PAGE_SHIFT;
 	}
@@ -113,7 +113,7 @@ int dram_init_banksize(void)
 	desc = map->desc;
 	for (num_banks = 0;
 	     desc < end && num_banks < CONFIG_NR_DRAM_BANKS;
-	     desc = efi_get_next_mem_desc(map, desc)) {
+	     desc = efi_get_next_mem_desc(desc, map->desc_size)) {
 		/*
 		 * We only use conventional memory and ignore
 		 * anything less than 1MB.
@@ -196,7 +196,7 @@ unsigned int install_e820_map(unsigned int max_entries,
 
 	end = (struct efi_mem_desc *)((ulong)map + size);
 	for (desc = map->desc; desc < end;
-	     desc = efi_get_next_mem_desc(map, desc)) {
+	     desc = efi_get_next_mem_desc(desc, map->desc_size)) {
 		if (desc->num_pages == 0)
 			continue;
 
diff --git a/cmd/efi.c b/cmd/efi.c
index f2ed26bd4b2..d2400acbbba 100644
--- a/cmd/efi.c
+++ b/cmd/efi.c
@@ -75,16 +75,17 @@ static int h_cmp_entry(const void *v1, const void *v2)
 /**
  * efi_build_mem_table() - make a sorted copy of the memory table
  *
- * @map:	Pointer to EFI memory map table
+ * @desc_base:	Pointer to EFI memory map table
  * @size:	Size of table in bytes
+ * @desc_size:	Size of each @desc_base record
  * @skip_bs:	True to skip boot-time memory and merge it with conventional
  *		memory. This will significantly reduce the number of table
  *		entries.
  * Return:	pointer to the new table. It should be freed with free() by the
  *		caller.
  */
-static void *efi_build_mem_table(struct efi_entry_memmap *map, int size,
-				 bool skip_bs)
+static void *efi_build_mem_table(struct efi_mem_desc *desc_base, int size,
+				 int desc_size, bool skip_bs)
 {
 	struct efi_mem_desc *desc, *end, *base, *dest, *prev;
 	int count;
@@ -95,15 +96,16 @@ static void *efi_build_mem_table(struct efi_entry_memmap *map, int size,
 		debug("%s: Cannot allocate %#x bytes\n", __func__, size);
 		return NULL;
 	}
-	end = (struct efi_mem_desc *)((ulong)map + size);
-	count = ((ulong)end - (ulong)map->desc) / map->desc_size;
-	memcpy(base, map->desc, (ulong)end - (ulong)map->desc);
-	qsort(base, count, map->desc_size, h_cmp_entry);
+	end = (void *)desc_base + size;
+	count = ((ulong)end - (ulong)desc_base) / desc_size;
+	memcpy(base, desc_base, (ulong)end - (ulong)desc_base);
+	qsort(base, count, desc_size, h_cmp_entry);
 	prev = NULL;
 	addr = 0;
 	dest = base;
-	end = (struct efi_mem_desc *)((ulong)base + count * map->desc_size);
-	for (desc = base; desc < end; desc = efi_get_next_mem_desc(map, desc)) {
+	end = (struct efi_mem_desc *)((ulong)base + count * desc_size);
+	for (desc = base; desc < end;
+	     desc = efi_get_next_mem_desc(desc, desc_size)) {
 		bool merge = true;
 		u32 type = desc->type;
 
@@ -116,7 +118,7 @@ static void *efi_build_mem_table(struct efi_entry_memmap *map, int size,
 		if (skip_bs && is_boot_services(desc->type))
 			type = EFI_CONVENTIONAL_MEMORY;
 
-		memcpy(dest, desc, map->desc_size);
+		memcpy(dest, desc, desc_size);
 		dest->type = type;
 		if (!skip_bs || !prev)
 			merge = false;
@@ -131,7 +133,7 @@ static void *efi_build_mem_table(struct efi_entry_memmap *map, int size,
 			prev->num_pages += desc->num_pages;
 		} else {
 			prev = dest;
-			dest = efi_get_next_mem_desc(map, dest);
+			dest = efi_get_next_mem_desc(dest, desc_size);
 		}
 		addr = desc->physical_start + (desc->num_pages <<
 				EFI_PAGE_SHIFT);
@@ -143,8 +145,8 @@ static void *efi_build_mem_table(struct efi_entry_memmap *map, int size,
 	return base;
 }
 
-static void efi_print_mem_table(struct efi_entry_memmap *map,
-				struct efi_mem_desc *desc, bool skip_bs)
+static void efi_print_mem_table(struct efi_mem_desc *desc, int desc_size,
+				bool skip_bs)
 {
 	u64 attr_seen[ATTR_SEEN_MAX];
 	int attr_seen_count;
@@ -158,7 +160,7 @@ static void efi_print_mem_table(struct efi_entry_memmap *map,
 	attr_seen_count = 0;
 	addr = 0;
 	for (upto = 0; desc->type != EFI_MAX_MEMORY_TYPE;
-	     upto++, desc = efi_get_next_mem_desc(map, desc)) {
+	     upto++, desc = efi_get_next_mem_desc(desc, desc_size)) {
 		const char *name;
 		u64 size;
 
@@ -238,13 +240,13 @@ static int do_efi_mem(struct cmd_tbl *cmdtp, int flag, int argc,
 		goto done;
 	}
 
-	desc = efi_build_mem_table(map, size, skip_bs);
+	desc = efi_build_mem_table(map->desc, size, map->desc_size, skip_bs);
 	if (!desc) {
 		ret = -ENOMEM;
 		goto done;
 	}
 
-	efi_print_mem_table(map, desc, skip_bs);
+	efi_print_mem_table(desc, map->desc_size, skip_bs);
 	free(desc);
 done:
 	if (ret)
diff --git a/include/efi.h b/include/efi.h
index dbf4ffaaf02..b29c49b2dcb 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -395,9 +395,9 @@ struct efi_entry_systable {
 };
 
 static inline struct efi_mem_desc *efi_get_next_mem_desc(
-		struct efi_entry_memmap *map, struct efi_mem_desc *desc)
+		struct efi_mem_desc *desc, int desc_size)
 {
-	return (struct efi_mem_desc *)((ulong)desc + map->desc_size);
+	return (struct efi_mem_desc *)((ulong)desc + desc_size);
 }
 
 /**
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 18/25] efi: Support the efi command in the app

[Simon Glass]

At present the 'efi' command only works in the EFI payload. Update it to
work in the app too, so the memory map can be examined.

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

(no changes since v1)

 cmd/Makefile      |  2 +-
 cmd/efi.c         | 48 ++++++++++++++++++++++++++++++++---------------
 include/efi.h     | 15 +++++++++++++++
 lib/efi/efi_app.c | 33 ++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 16 deletions(-)

diff --git a/cmd/Makefile b/cmd/Makefile
index e31ac15ef75..6623d7eaa05 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -58,7 +58,7 @@ obj-$(CONFIG_CMD_EXTENSION) += extension_board.o
 obj-$(CONFIG_CMD_ECHO) += echo.o
 obj-$(CONFIG_ENV_IS_IN_EEPROM) += eeprom.o
 obj-$(CONFIG_CMD_EEPROM) += eeprom.o
-obj-$(CONFIG_EFI_STUB) += efi.o
+obj-$(CONFIG_EFI) += efi.o
 obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o
 obj-$(CONFIG_CMD_ELF) += elf.o
 obj-$(CONFIG_HUSH_PARSER) += exit.o
diff --git a/cmd/efi.c b/cmd/efi.c
index d2400acbbba..c0384e0db28 100644
--- a/cmd/efi.c
+++ b/cmd/efi.c
@@ -13,6 +13,8 @@
 #include <sort.h>
 #include <asm/global_data.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 static const char *const type_name[] = {
 	"reserved",
 	"loader_code",
@@ -217,37 +219,53 @@ static void efi_print_mem_table(struct efi_mem_desc *desc, int desc_size,
 static int do_efi_mem(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[])
 {
-	struct efi_mem_desc *desc;
-	struct efi_entry_memmap *map;
+	struct efi_mem_desc *orig, *desc;
+	uint version, key;
+	int desc_size;
 	int size, ret;
 	bool skip_bs;
 
 	skip_bs = !argc || *argv[0] != 'a';
-	ret = efi_info_get(EFIET_MEMORY_MAP, (void **)&map, &size);
-	switch (ret) {
-	case -ENOENT:
-		printf("No EFI table available\n");
-		goto done;
-	case -EPROTONOSUPPORT:
-		printf("Incorrect EFI table version\n");
-		goto done;
+	if (IS_ENABLED(CONFIG_EFI_APP)) {
+		ret = efi_get_mmap(&orig, &size, &key, &desc_size, &version);
+		if (ret) {
+			printf("Cannot read memory map (err=%d)\n", ret);
+			return CMD_RET_FAILURE;
+		}
+	} else {
+		struct efi_entry_memmap *map;
+
+		ret = efi_info_get(EFIET_MEMORY_MAP, (void **)&map, &size);
+		switch (ret) {
+		case -ENOENT:
+			printf("No EFI table available\n");
+			goto done;
+		case -EPROTONOSUPPORT:
+			printf("Incorrect EFI table version\n");
+			goto done;
+		}
+		orig = map->desc;
+		desc_size = map->desc_size;
+		version = map->version;
 	}
-	printf("EFI table at %lx, memory map %p, size %x, version %x, descr. size %#x\n",
-	       gd->arch.table, map, size, map->version, map->desc_size);
-	if (map->version != EFI_MEM_DESC_VERSION) {
+	printf("EFI table at %lx, memory map %p, size %x, key %x, version %x, descr. size %#x\n",
+	       gd->arch.table, orig, size, key, version, desc_size);
+	if (version != EFI_MEM_DESC_VERSION) {
 		printf("Incorrect memory map version\n");
 		ret = -EPROTONOSUPPORT;
 		goto done;
 	}
 
-	desc = efi_build_mem_table(map->desc, size, map->desc_size, skip_bs);
+	desc = efi_build_mem_table(orig, size, desc_size, skip_bs);
 	if (!desc) {
 		ret = -ENOMEM;
 		goto done;
 	}
 
-	efi_print_mem_table(desc, map->desc_size, skip_bs);
+	efi_print_mem_table(desc, desc_size, skip_bs);
 	free(desc);
+	if (IS_ENABLED(CONFIG_EFI_APP))
+		free(orig);
 done:
 	if (ret)
 		printf("Error: %d\n", ret);
diff --git a/include/efi.h b/include/efi.h
index b29c49b2dcb..751dee876e7 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -610,4 +610,19 @@ int efi_store_memory_map(struct efi_priv *priv);
  */
 int efi_call_exit_boot_services(void);
 
+/**
+ * efi_get_mmap() - Get the memory map from EFI
+ *
+ * This is used in the app. The caller must free *@descp when done
+ *
+ * @descp:	Returns allocated pointer to EFI memory map table
+ * @sizep:	Returns size of table in bytes
+ * @keyp:	Returns memory-map key
+ * @desc_sizep:	Returns size of each @desc_base record
+ * @versionp:	Returns version number of memory map
+ * @return 0 on success, -ve on error
+ */
+int efi_get_mmap(struct efi_mem_desc **descp, int *sizep, uint *keyp,
+		 int *desc_sizep, uint *versionp);
+
 #endif /* _LINUX_EFI_H */
diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index f5eab0823cf..8761fcedb31 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -32,6 +32,39 @@ int efi_info_get(enum efi_entry_t type, void **datap, int *sizep)
 	return -ENOSYS;
 }
 
+int efi_get_mmap(struct efi_mem_desc **descp, int *sizep, uint *keyp,
+		 int *desc_sizep, uint *versionp)
+{
+	struct efi_priv *priv = efi_get_priv();
+	struct efi_boot_services *boot = priv->sys_table->boottime;
+	efi_uintn_t size, desc_size, key;
+	struct efi_mem_desc *desc;
+	efi_status_t ret;
+	u32 version;
+
+	/* Get the memory map so we can switch off EFI */
+	size = 0;
+	ret = boot->get_memory_map(&size, NULL, &key, &desc_size, &version);
+	if (ret != EFI_BUFFER_TOO_SMALL)
+		return log_msg_ret("get", -ENOMEM);
+
+	desc = malloc(size);
+	if (!desc)
+		return log_msg_ret("mem", -ENOMEM);
+
+	ret = boot->get_memory_map(&size, desc, &key, &desc_size, &version);
+	if (ret)
+		return log_msg_ret("get", -EINVAL);
+
+	*descp = desc;
+	*sizep = size;
+	*desc_sizep = desc_size;
+	*versionp = version;
+	*keyp = key;
+
+	return 0;
+}
+
 /**
  * efi_bind_block() - bind a new block device to an EFI device
  *
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 19/25] x86: efi: Show the system-table revision

[Simon Glass]

Show the revision of this table as it can be important.

Also update the 'efi table' entry to show the actual address of the EFI
table rather than our table that points to it. This saves a step and the
intermediate table has nothing else in it.

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

(no changes since v6)

Changes in v6:
- Fix Alo typo in commit message

Changes in v5:
- Fix grammar in commit message

Changes in v3:
- Add new patch to show the system-table revision

 arch/x86/cpu/efi/payload.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/cpu/efi/payload.c b/arch/x86/cpu/efi/payload.c
index d2aa889a2b9..b7778565b19 100644
--- a/arch/x86/cpu/efi/payload.c
+++ b/arch/x86/cpu/efi/payload.c
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <cpu_func.h>
 #include <efi.h>
+#include <efi_api.h>
 #include <errno.h>
 #include <init.h>
 #include <log.h>
@@ -296,8 +297,14 @@ void setup_efi_info(struct efi_info *efi_info)
 void efi_show_bdinfo(void)
 {
 	struct efi_entry_systable *table = NULL;
+	struct efi_system_table *sys_table;
 	int size, ret;
 
 	ret = efi_info_get(EFIET_SYS_TABLE, (void **)&table, &size);
-	bdinfo_print_num_l("efi_table", (ulong)table);
+	if (!ret) {
+		bdinfo_print_num_l("efi_table", table->sys_table);
+		sys_table = (struct efi_system_table *)(uintptr_t)
+			table->sys_table;
+		bdinfo_print_num_l(" revision", sys_table->fw_revision);
+	}
 }
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 20/25] x86: efi: Don't set up global_data again with EFI

[Simon Glass]

Since EFI does not relocate and uses the same global_data pointer
throughout the board-init process, drop this unnecessary setup, to avoid
a hang.

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

(no changes since v5)

Changes in v5:
- Add new patch to avoid setting up global_data again with EFI

 common/board_r.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/common/board_r.c b/common/board_r.c
index 31a59c585a8..8b5948100b1 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -817,9 +817,8 @@ void board_init_r(gd_t *new_gd, ulong dest_addr)
 	 * TODO(sjg@chromium.org): Consider doing this for all archs, or
 	 * dropping the new_gd parameter.
 	 */
-#if CONFIG_IS_ENABLED(X86_64)
-	arch_setup_gd(new_gd);
-#endif
+	if (CONFIG_IS_ENABLED(X86_64) && !IS_ENABLED(CONFIG_EFI_APP))
+		arch_setup_gd(new_gd);
 
 #ifdef CONFIG_NEEDS_MANUAL_RELOC
 	int i;
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 21/25] x86: efi: Tweak the code used for the 64-bit EFI app

[Simon Glass]

Add an empty CPU init function to avoid fiddling with low-level CPU
features in the app. Set up the C runtime correctly for 64-bit use
and avoid clearing BSS, since this is done by EFI when U-Boot is loaded.

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

(no changes since v5)

Changes in v5:
- Add new patch to tweak the code used for the 64-bit EFI app

 arch/x86/cpu/x86_64/cpu.c | 5 +++++
 arch/x86/lib/Makefile     | 5 ++---
 arch/x86/lib/relocate.c   | 2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
index a3674e8e29a..6a387612916 100644
--- a/arch/x86/cpu/x86_64/cpu.c
+++ b/arch/x86/cpu/x86_64/cpu.c
@@ -45,3 +45,8 @@ int cpu_phys_address_size(void)
 {
 	return CONFIG_CPU_ADDR_BITS;
 }
+
+int x86_cpu_init_f(void)
+{
+	return 0;
+}
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index 18757b29aa9..e5235b7c4f4 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -65,9 +65,8 @@ endif
 
 lib-$(CONFIG_USE_PRIVATE_LIBGCC) += div64.o
 
-ifeq ($(CONFIG_$(SPL_)X86_64),)
-obj-$(CONFIG_EFI_APP) += crt0_ia32_efi.o reloc_ia32_efi.o
-endif
+obj-$(CONFIG_EFI_APP_32BIT) += crt0_ia32_efi.o reloc_ia32_efi.o
+obj-$(CONFIG_EFI_APP_64BIT) += crt0_x86_64_efi.o reloc_x86_64_efi.o
 
 ifneq ($(CONFIG_EFI_STUB),)
 
diff --git a/arch/x86/lib/relocate.c b/arch/x86/lib/relocate.c
index 6fe51516477..9060d19d46a 100644
--- a/arch/x86/lib/relocate.c
+++ b/arch/x86/lib/relocate.c
@@ -35,6 +35,7 @@ int copy_uboot_to_ram(void)
 	return 0;
 }
 
+#ifndef CONFIG_EFI_APP
 int clear_bss(void)
 {
 	ulong dst_addr = (ulong)&__bss_start + gd->reloc_off;
@@ -46,6 +47,7 @@ int clear_bss(void)
 
 	return 0;
 }
+#endif
 
 #if CONFIG_IS_ENABLED(X86_64)
 static void do_elf_reloc_fixups64(unsigned int text_base, uintptr_t size,
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 22/25] x86: efi: Round out the link script for 64-bit EFI

[Simon Glass]

Make sure the linker lists are in the right place and drop the eh_frame
section, which is not needed.

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

(no changes since v5)

Changes in v5:
- Add new patch to round out the link script for 64-bit EFI

 arch/x86/lib/elf_x86_64_efi.lds | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/lib/elf_x86_64_efi.lds b/arch/x86/lib/elf_x86_64_efi.lds
index b436429b33e..75727400aa4 100644
--- a/arch/x86/lib/elf_x86_64_efi.lds
+++ b/arch/x86/lib/elf_x86_64_efi.lds
@@ -63,6 +63,7 @@ SECTIONS
 		*(.rela.data*)
 		*(.rela.got)
 		*(.rela.stab)
+                *(.rela.u_boot_list*)
 	}
 
 	. = ALIGN(4096);
@@ -70,9 +71,11 @@ SECTIONS
 	. = ALIGN(4096);
 	.dynstr : { *(.dynstr) }
 	. = ALIGN(4096);
+
+        /DISCARD/ : { *(.eh_frame) }
+
 	.ignored.reloc : {
 		*(.rela.reloc)
-		*(.eh_frame)
 		*(.note.GNU-stack)
 	}
 
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 23/25] x86: efi: Don't use the 64-bit link script for the EFI app

[Simon Glass]

That script is not intended for use with EFI, so update the logic to avoid
using it.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Christian Melki <christian.melki@t2data.com>
---

(no changes since v5)

Changes in v5:
- Add new patch to avoid using the 64-bit link script for the EFI app

 arch/x86/cpu/config.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/cpu/config.mk b/arch/x86/cpu/config.mk
index d3033b41603..87e242a2065 100644
--- a/arch/x86/cpu/config.mk
+++ b/arch/x86/cpu/config.mk
@@ -9,7 +9,7 @@ LDPPFLAGS += -DRESET_VEC_LOC=$(CONFIG_RESET_VEC_LOC)
 LDPPFLAGS += -DSTART_16=$(CONFIG_SYS_X86_START16)
 
 ifdef CONFIG_X86_64
-ifndef CONFIG_SPL_BUILD
+ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_EFI_APP),)
 LDSCRIPT = $(srctree)/arch/x86/cpu/u-boot-64.lds
 endif
 endif
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 24/25] x86: efi: Set the correct link flags for the 64-bit EFI app

[Simon Glass]

At present some 32-bit settings are used with the 64-bit app. Fix this by
separating out the two cases.

Be careful not to break the 64-bit payload, which needs to build a 64-bit
EFI stub with a 32-bit U-Boot.

Signed-off-by: Christian Melki <christian.melki@t2data.com>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

(no changes since v5)

Changes in v5:
- Add new patch to set the correct link flags for the 64-bit EFI app

 arch/x86/config.mk | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index 589f2aed2bc..889497b6bd7 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -20,6 +20,11 @@ IS_32BIT := y
 endif
 endif
 
+EFI_IS_32BIT := $(IS_32BIT)
+ifdef CONFIG_EFI_STUB_64BIT
+EFI_IS_32BIT :=
+endif
+
 ifeq ($(IS_32BIT),y)
 PLATFORM_CPPFLAGS += -march=i386 -m32
 else
@@ -44,8 +49,14 @@ CFLAGS_EFI := -fpic -fshort-wchar
 # Compiler flags to be removed when building UEFI applications
 CFLAGS_NON_EFI := -mregparm=3 -fstack-protector-strong
 
-ifeq ($(CONFIG_EFI_STUB_64BIT),)
+ifeq ($(IS_32BIT),y)
+EFIPAYLOAD_BFDARCH = i386
+else
 CFLAGS_EFI += $(call cc-option, -mno-red-zone)
+EFIPAYLOAD_BFDARCH = x86_64
+endif
+
+ifeq ($(EFI_IS_32BIT),y)
 EFIARCH = ia32
 EFIPAYLOAD_BFDTARGET = elf32-i386
 else
@@ -53,8 +64,6 @@ EFIARCH = x86_64
 EFIPAYLOAD_BFDTARGET = elf64-x86-64
 endif
 
-EFIPAYLOAD_BFDARCH = i386
-
 LDSCRIPT_EFI := $(srctree)/arch/x86/lib/elf_$(EFIARCH)_efi.lds
 EFISTUB := crt0_$(EFIARCH)_efi.o reloc_$(EFIARCH)_efi.o
 OBJCOPYFLAGS_EFI += --target=efi-app-$(EFIARCH)
-- 
2.34.1.448.ga2b2bfdf31-goog

[PATCH v8 25/25] efi: Build the 64-bit app properly

[Simon Glass]

Now that the linker crash is resolved, build the 64-bit EFI app, including
all the required code.

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

(no changes since v7)

Changes in v7:
- Rebase on -master instead of -next

Changes in v5:
- Add new patch to build the 64-bit app properly
- Add various patches to fix up the 64-bit app so that it boots

Changes in v2:
- Add new patch to support the efi command in the app

 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index ae9bfab91ac..3d5388b1254 100644
--- a/Makefile
+++ b/Makefile
@@ -1764,9 +1764,9 @@ else
 quiet_cmd_u-boot__ ?= LD      $@
       cmd_u-boot__ ?= $(LD) $(KBUILD_LDFLAGS) $(LDFLAGS_u-boot) -o $@		\
 		-T u-boot.lds $(u-boot-init)					\
-		$(if $(CONFIG_EFI_APP_64BIT),,--whole-archive)			\
+		--whole-archive							\
 			$(u-boot-main)						\
-		$(if $(CONFIG_EFI_APP_64BIT),,--no-whole-archive)		\
+		--no-whole-archive						\
 		$(PLATFORM_LIBS) -Map u-boot.map;				\
 		$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
 endif
-- 
2.34.1.448.ga2b2bfdf31-goog

Re: [PATCH v8 01/25] efi: Make unicode printf available to the app

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> This is needed to show unicode strings. Enable this code in the app.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Re: [PATCH v8 02/25] efi: Locate all block devices in the app

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> When starting the app, locate all block devices and make them available
> to U-Boot. This allows listing partitions and accessing files in
> filesystems.
>
> EFI also has the concept of 'disks', meaning boot media. For now, this
> is not obviously useful in U-Boot, but add code to at least locate these.
> This can be expanded later as needed.
>
> We cannot use printf() in the early stub or app since it is not compiled
> in
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v8:
> - Use the unicode print functions instead of rolling out own
>
> Changes in v6:
> - Add comment for dm_scan_other()
> - Add comment for free_memory()
> - Drop setup_disks() as U-Boot does not support it
> - Fix 'have have' typo
> - Update comment format for devpath_is_partition()
>
> Changes in v2:
> - Don't export efi_bind_block()
> - Only bind devices for media devices, not for partitions
> - Show devices that are processed
> - Store device path in struct efi_media_plat
> - Update documentation
>
>   doc/develop/uefi/u-boot_on_efi.rst |   4 +-
>   include/efi.h                      |   6 +-
>   include/efi_api.h                  |  15 +++
>   lib/efi/efi_app.c                  | 197 +++++++++++++++++++++++++++++
>   4 files changed, 217 insertions(+), 5 deletions(-)
>
> diff --git a/doc/develop/uefi/u-boot_on_efi.rst b/doc/develop/uefi/u-boot_on_efi.rst
> index 5f2f850f071..8f81b799072 100644
> --- a/doc/develop/uefi/u-boot_on_efi.rst
> +++ b/doc/develop/uefi/u-boot_on_efi.rst
> @@ -265,9 +265,7 @@ This work could be extended in a number of ways:
>
>   - Figure out how to solve the interrupt problem
>
> -- Add more drivers to the application side (e.g. block devices, USB,
> -  environment access). This would mostly be an academic exercise as a strong
> -  use case is not readily apparent, but it might be fun.
> +- Add more drivers to the application side (e.g.USB, environment access).
>
>   - Avoid turning off boot services in the stub. Instead allow U-Boot to make
>     use of boot services in case it wants to. It is unclear what it might want
> diff --git a/include/efi.h b/include/efi.h
> index 1432038838a..cd0bdcc717b 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -419,10 +419,12 @@ struct efi_priv {
>    *
>    * @handle: handle of the controller on which this driver is installed
>    * @blkio: block io protocol proxied by this driver
> + * @device_path: EFI path to the device
>    */
>   struct efi_media_plat {
> -	efi_handle_t		handle;
> -	struct efi_block_io	*blkio;
> +	efi_handle_t handle;
> +	struct efi_block_io *blkio;
> +	struct efi_device_path *device_path;
>   };
>
>   /* Base address of the EFI image */
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 80109f012bc..ec9fa89a935 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -2035,4 +2035,19 @@ struct efi_firmware_management_protocol {
>   			const u16 *package_version_name);
>   };
>
> +#define EFI_DISK_IO_PROTOCOL_GUID	\
> +	EFI_GUID(0xce345171, 0xba0b, 0x11d2, 0x8e, 0x4f, \
> +		 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
> +
> +struct efi_disk {
> +	u64 revision;
> +	efi_status_t (EFIAPI *read_disk)(struct efi_disk *this, u32 media_id,
> +					 u64 offset, efi_uintn_t buffer_size,
> +					 void *buffer);
> +
> +	efi_status_t (EFIAPI *write_disk)(struct efi_disk *this, u32 media_id,
> +					  u64 offset, efi_uintn_t buffer_size,
> +					  void *buffer);
> +};
> +
>   #endif
> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> index f61665686c5..852cf3679d6 100644
> --- a/lib/efi/efi_app.c
> +++ b/lib/efi/efi_app.c
> @@ -21,6 +21,9 @@
>   #include <efi.h>
>   #include <efi_api.h>
>   #include <sysreset.h>
> +#include <dm/device-internal.h>
> +#include <dm/lists.h>
> +#include <dm/root.h>
>
>   DECLARE_GLOBAL_DATA_PTR;
>
> @@ -46,6 +49,49 @@ int efi_info_get(enum efi_entry_t type, void **datap, int *sizep)
>   	return -ENOSYS;
>   }
>
> +/**
> + * efi_bind_block() - bind a new block device to an EFI device
> + *
> + * Binds a new top-level EFI_MEDIA device as well as a child block device so
> + * that the block device can be accessed in U-Boot.
> + *
> + * The device can then be accessed using 'part list efi 0', 'fat ls efi 0:1',
> + * for example, just like any other interface type.
> + *
> + * @handle: handle of the controller on which this driver is installed
> + * @blkio: block io protocol proxied by this driver
> + * @device_path: EFI device path structure for this
> + * @len: Length of @device_path in bytes
> + * @devp: Returns the bound device
> + * @return 0 if OK, -ve on error
> + */
> +int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio,
> +		   struct efi_device_path *device_path, int len,
> +		   struct udevice **devp)
> +{
> +	struct efi_media_plat plat;
> +	struct udevice *dev;
> +	char name[18];
> +	int ret;
> +
> +	plat.handle = handle;
> +	plat.blkio = blkio;
> +	plat.device_path = malloc(device_path->length);
> +	if (!plat.device_path)
> +		return log_msg_ret("path", -ENOMEM);
> +	memcpy(plat.device_path, device_path, device_path->length);
> +	ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
> +			  &plat, ofnode_null(), &dev);
> +	if (ret)
> +		return log_msg_ret("bind", ret);
> +
> +	snprintf(name, sizeof(name), "efi_media_%x", dev_seq(dev));
> +	device_set_name(dev, name);
> +	*devp = dev;
> +
> +	return 0;
> +}
> +
>   static efi_status_t setup_memory(struct efi_priv *priv)
>   {
>   	struct efi_boot_services *boot = priv->boot;
> @@ -91,6 +137,14 @@ static efi_status_t setup_memory(struct efi_priv *priv)
>   	return 0;
>   }
>
> +/**
> + * free_memory() - Free memory used by the U-Boot app
> + *
> + * This frees memory allocated in setup_memory(), in preparation for returning
> + * to UEFI. It also zeroes the global_data pointer.
> + *
> + * @priv: Private EFI data
> + */
>   static void free_memory(struct efi_priv *priv)
>   {
>   	struct efi_boot_services *boot = priv->boot;
> @@ -105,6 +159,149 @@ static void free_memory(struct efi_priv *priv)
>   	global_data_ptr = NULL;
>   }
>
> +/**
> + * devpath_is_partition() - Figure out if a device path is a partition
> + *
> + * Checks if a device path refers to a partition on some media device. This
> + * works by checking for a valid partition number in a hard-driver media device
> + * as the final component of the device path.

The description of parameter @path is missing.

I can add this when merging.

Best regards

Heinrich

> + *
> + * Return: true if a partition, false if not (e.g. it might be media which
> + *	contains partitions)
> + */
> +static bool devpath_is_partition(const struct efi_device_path *path)
> +{
> +	const struct efi_device_path *p;
> +	bool was_part;
> +
> +	for (p = path; p->type != DEVICE_PATH_TYPE_END;
> +	     p = (void *)p + p->length) {
> +		was_part = false;
> +		if (p->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
> +		    p->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
> +			struct efi_device_path_hard_drive_path *hd =
> +				(void *)path;
> +
> +			if (hd->partition_number)
> +				was_part = true;
> +		}
> +	}
> +
> +	return was_part;
> +}
> +
> +/**
> + * setup_block() - Find all block devices and setup EFI devices for them
> + *
> + * Partitions are ignored, since U-Boot has partition handling. Errors with
> + * particular devices produce a warning but execution continues to try to
> + * find others.
> + *
> + * Return: 0 if found, -ENOSYS if there is no boot-services table, -ENOTSUPP
> + *	if a required protocol is not supported
> + */
> +static int setup_block(void)
> +{
> +	efi_guid_t efi_blkio_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
> +	efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
> +	efi_guid_t efi_pathutil_guid = EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID;
> +	efi_guid_t efi_pathtext_guid = EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
> +	struct efi_boot_services *boot = efi_get_boot();
> +	struct efi_device_path_utilities_protocol *util;
> +	struct efi_device_path_to_text_protocol *text;
> +	struct efi_device_path *path;
> +	struct efi_block_io *blkio;
> +	efi_uintn_t num_handles;
> +	efi_handle_t *handle;
> +	int ret, i;
> +
> +	if (!boot)
> +		return log_msg_ret("sys", -ENOSYS);
> +
> +	/* Find all devices which support the block I/O protocol */
> +	ret = boot->locate_handle_buffer(BY_PROTOCOL, &efi_blkio_guid, NULL,
> +				  &num_handles, &handle);
> +	if (ret)
> +		return log_msg_ret("loc", -ENOTSUPP);
> +	log_debug("Found %d handles:\n", (int)num_handles);
> +
> +	/* We need to look up the path size and convert it to text */
> +	ret = boot->locate_protocol(&efi_pathutil_guid, NULL, (void **)&util);
> +	if (ret)
> +		return log_msg_ret("util", -ENOTSUPP);
> +	ret = boot->locate_protocol(&efi_pathtext_guid, NULL, (void **)&text);
> +	if (ret)
> +		return log_msg_ret("text", -ENOTSUPP);
> +
> +	for (i = 0; i < num_handles; i++) {
> +		struct udevice *dev;
> +		const u16 *name;
> +		bool is_part;
> +		int len;
> +
> +		ret = boot->handle_protocol(handle[i], &efi_devpath_guid,
> +					    (void **)&path);
> +		if (ret) {
> +			log_warning("- devpath %d failed (ret=%d)\n", i, ret);
> +			continue;
> +		}
> +
> +		ret = boot->handle_protocol(handle[i], &efi_blkio_guid,
> +					    (void **)&blkio);
> +		if (ret) {
> +			log_warning("- blkio %d failed (ret=%d)\n", i, ret);
> +			continue;
> +		}
> +
> +		name = text->convert_device_path_to_text(path, true, false);
> +		is_part = devpath_is_partition(path);
> +
> +		if (!is_part) {
> +			len = util->get_device_path_size(path);
> +			ret = efi_bind_block(handle[i], blkio, path, len, &dev);
> +			if (ret) {
> +				log_warning("- blkio bind %d failed (ret=%d)\n",
> +					    i, ret);
> +				continue;
> +			}
> +		} else {
> +			dev = NULL;
> +		}
> +
> +		/*
> +		 * Show the device name if we created one. Otherwise indicate
> +		 * that it is a partition.
> +		 */
> +		printf("%2d: %-12s %ls\n", i, dev ? dev->name : "<partition>",
> +		       name);
> +	}
> +	boot->free_pool(handle);
> +
> +	return 0;
> +}
> +
> +/**
> + * dm_scan_other() - Scan for UEFI devices that should be available to U-Boot
> + *
> + * This sets up block devices within U-Boot for those found in UEFI. With this,
> + * U-Boot can access those devices
> + *
> + * @pre_reloc_only: true to only bind pre-relocation devices (ignored)
> + * Returns: 0 on success, -ve on error
> + */
> +int dm_scan_other(bool pre_reloc_only)
> +{
> +	if (gd->flags & GD_FLG_RELOC) {
> +		int ret;
> +
> +		ret = setup_block();
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * efi_main() - Start an EFI image
>    *

Re: [PATCH v8 05/25] x86: Don't process the kernel command line unless enabled

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> If the 'bootm' command is not enabled then this code is not available and
> this causes a link error. Fix it.
>
> Note that for the EFI app, there is no indication of missing code. It just
> hangs!
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>   arch/x86/lib/zimage.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
> index 7ce02226ef9..9cc04490307 100644
> --- a/arch/x86/lib/zimage.c
> +++ b/arch/x86/lib/zimage.c
> @@ -365,11 +365,14 @@ int setup_zimage(struct boot_params *setup_base, char *cmd_line, int auto_boot,
>   			strcpy(cmd_line, (char *)cmdline_force);
>   		else
>   			build_command_line(cmd_line, auto_boot);
> -		ret = bootm_process_cmdline(cmd_line, max_size, BOOTM_CL_ALL);
> -		if (ret) {
> -			printf("Cmdline setup failed (max_size=%x, bootproto=%x, err=%d)\n",
> -			       max_size, bootproto, ret);
> -			return ret;
> +		if (IS_ENABLED(CONFIG_CMD_BOOTM)) {

boot/Makefile:
obj-$(CONFIG_CMD_BOOTM) += bootm.o bootm_os.o
obj-$(CONFIG_CMD_BOOTZ) += bootm.o bootm_os.o
obj-$(CONFIG_CMD_BOOTI) += bootm.o bootm_os.o

Why don't you check for CONFIG_CMD_BOOTZ too (booti is not applicable to
x86)?

if (IS_ENABLED(CONFIG_CMD_BOOTM) || IS_ENABLED(CONFIG_CMD_BOOTZ)) {

Best regards

Heinrich

> +			ret = bootm_process_cmdline(cmd_line, max_size,
> +						    BOOTM_CL_ALL);
> +			if (ret) {
> +				printf("Cmdline setup failed (max_size=%x, bootproto=%x, err=%d)\n",
> +				       max_size, bootproto, ret);
> +				return ret;
> +			}
>   		}
>   		printf("Kernel command line: \"");
>   		puts(cmd_line);

Re: [PATCH v8 06/25] x86: efi: Add room for the binman definition in the dtb

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> At present only 4KB of spare space is left in the DTB when building the
> EFI app. Increase this to 32KB so there is plenty of space to insert the
> binman definition. This cannot be expanded later (as with OF_SEPARATE)
> because the ELF image has already been built.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v1)
>
>   arch/x86/dts/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/dts/Makefile b/arch/x86/dts/Makefile
> index be209aaaf8f..5c8c05ec499 100644
> --- a/arch/x86/dts/Makefile
> +++ b/arch/x86/dts/Makefile
> @@ -24,7 +24,7 @@ dtb-y += bayleybay.dtb \
>
>   targets += $(dtb-y)
>
> -DTC_FLAGS += -R 4 -p 0x1000
> +DTC_FLAGS += -R 4 -p $(if $(CONFIG_EFI_APP),0x8000,0x1000)
>
>   PHONY += dtbs
>   dtbs: $(addprefix $(obj)/, $(dtb-y))

Re: [PATCH v8 07/25] efi: Drop device_path from struct efi_priv

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> This is not used anywhere drop it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Move device_path path change to its own patch
>
>   include/efi.h | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/include/efi.h b/include/efi.h
> index cd0bdcc717b..0cd4b46600e 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -402,7 +402,6 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
>
>   struct efi_priv {
>   	efi_handle_t parent_image;
> -	struct efi_device_path *device_path;
>   	struct efi_system_table *sys_table;
>   	struct efi_boot_services *boot;
>   	struct efi_runtime_services *run;

Re: [PATCH v8 05/25] x86: Don't process the kernel command line unless enabled

[Simon Glass]

Hi Heinrich,

On Thu, 30 Dec 2021 at 06:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12/29/21 19:57, Simon Glass wrote:
> > If the 'bootm' command is not enabled then this code is not available and
> > this causes a link error. Fix it.
> >
> > Note that for the EFI app, there is no indication of missing code. It just
> > hangs!
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >   arch/x86/lib/zimage.c | 13 ++++++++-----
> >   1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
> > index 7ce02226ef9..9cc04490307 100644
> > --- a/arch/x86/lib/zimage.c
> > +++ b/arch/x86/lib/zimage.c
> > @@ -365,11 +365,14 @@ int setup_zimage(struct boot_params *setup_base, char *cmd_line, int auto_boot,
> >                       strcpy(cmd_line, (char *)cmdline_force);
> >               else
> >                       build_command_line(cmd_line, auto_boot);
> > -             ret = bootm_process_cmdline(cmd_line, max_size, BOOTM_CL_ALL);
> > -             if (ret) {
> > -                     printf("Cmdline setup failed (max_size=%x, bootproto=%x, err=%d)\n",
> > -                            max_size, bootproto, ret);
> > -                     return ret;
> > +             if (IS_ENABLED(CONFIG_CMD_BOOTM)) {
>
> boot/Makefile:
> obj-$(CONFIG_CMD_BOOTM) += bootm.o bootm_os.o
> obj-$(CONFIG_CMD_BOOTZ) += bootm.o bootm_os.o
> obj-$(CONFIG_CMD_BOOTI) += bootm.o bootm_os.o
>
> Why don't you check for CONFIG_CMD_BOOTZ too (booti is not applicable to
> x86)?
>
> if (IS_ENABLED(CONFIG_CMD_BOOTM) || IS_ENABLED(CONFIG_CMD_BOOTZ)) {
>

I think the real fix is what Tom suggested, which I can do later.

Regards,
Simon



> Best regards
>
> Heinrich
>
> > +                     ret = bootm_process_cmdline(cmd_line, max_size,
> > +                                                 BOOTM_CL_ALL);
> > +                     if (ret) {
> > +                             printf("Cmdline setup failed (max_size=%x, bootproto=%x, err=%d)\n",
> > +                                    max_size, bootproto, ret);
> > +                             return ret;
> > +                     }
> >               }
> >               printf("Kernel command line: \"");
> >               puts(cmd_line);
>

Re: [PATCH v8 08/25] efi: Add comments to struct efi_priv

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> This structure is uncommented. Fix it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Drop comments that confuse sphinx
> - Move device_path path change to its own patch
>
>   include/efi.h | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
>
> diff --git a/include/efi.h b/include/efi.h
> index 0cd4b46600e..57ca2f424ab 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -400,14 +400,37 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
>   	return (struct efi_mem_desc *)((ulong)desc + map->desc_size);
>   }
>
> +/**
> + * struct efi_priv - Information about the environment provided by EFI
> + *
> + * @parent_image: image passed into the EFI app or stub
> + * @sys_table: Pointer to system table
> + * @boot: Pointer to boot-services table
> + * @run: Pointer to runtime-services table
> + *
> + * @use_pool_for_malloc: true if all allocation should go through the EFI 'pool'
> + *	methods allocate_pool() and free_pool(); false to use 'pages' methods
> + *	allocate_pages() and free_pages()
> + * @ram_base: Base address of RAM (size CONFIG_EFI_RAM_SIZE)
> + * @image_data_type: Type of the loaded image (e.g. EFI_LOADER_CODE)
> + *
> + * @info: Header of the info list, holding info collected by the stub and passed
> + *	to U-Boot
> + * @info_size: Size of the info list, in bytes from @info

I guess you mean:
"@info_size: Size of the info list @info in bytes"

Best regards

Heinrich

> + * @next_hdr: Pointer to where to put the next header when adding to the list
> + */
>   struct efi_priv {
>   	efi_handle_t parent_image;
>   	struct efi_system_table *sys_table;
>   	struct efi_boot_services *boot;
>   	struct efi_runtime_services *run;
> +
> +	/* app: */
>   	bool use_pool_for_malloc;
>   	unsigned long ram_base;
>   	unsigned int image_data_type;
> +
> +	/* stub: */
>   	struct efi_info_hdr *info;
>   	unsigned int info_size;
>   	void *next_hdr;

Re: [PATCH v8 09/25] efi: Fix ll_boot_init() operation with the app

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> This should return false when the EFI app is running, since UEFI has done
> the required low-level init. Fix it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Re: [PATCH v8 10/25] efi: Add a few comments to the stub

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> Comment some functions that need more information.
>
> Signed-off-by: Simon Glass<sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Re: [PATCH v8 11/25] efi: Share struct efi_priv between the app and stub code

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> At present each of these has its own static variable and helper functions.
> Move them into a shared file.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>   include/efi.h      | 21 +++++++++++++++++++++
>   lib/efi/efi.c      | 29 +++++++++++++++++++++++++++++
>   lib/efi/efi_app.c  | 21 ++-------------------
>   lib/efi/efi_stub.c |  7 ++++---
>   4 files changed, 56 insertions(+), 22 deletions(-)
>
> diff --git a/include/efi.h b/include/efi.h
> index 57ca2f424ab..d4785478585 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -474,6 +474,27 @@ extern char _binary_u_boot_bin_start[], _binary_u_boot_bin_end[];
>   				EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS | \
>   				EFI_VARIABLE_APPEND_WRITE)
>
> +/**
> + * efi_get_priv() - Get access to the EFI-private information
> + *
> + * This struct it used by both the stub and the app to record things about the
> + * EFI environment. It is not available in U-Boot proper after the stub has
> + * jumped there. Use efi_info_get() to obtain info in that case.
> + *
> + * @return pointer to private info

%s/@return/Return:/

See
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

I will correct this when merging.

Best regards

Heinrich

> + */
> +struct efi_priv *efi_get_priv(void);
> +
> +/**
> + * efi_set_priv() - Set up a pointer to the EFI-private information
> + *
> + * This is called in the stub and app to record the location of this
> + * information.
> + *
> + * @priv: New location of private data
> + */
> +void efi_set_priv(struct efi_priv *priv);
> +
>   /**
>    * efi_get_sys_table() - Get access to the main EFI system table
>    *
> diff --git a/lib/efi/efi.c b/lib/efi/efi.c
> index 69e52e45748..cd6bf47b180 100644
> --- a/lib/efi/efi.c
> +++ b/lib/efi/efi.c
> @@ -1,5 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0+
>   /*
> + * Functions shared by the app and stub
> + *
>    * Copyright (c) 2015 Google, Inc
>    *
>    * EFI information obtained here:
> @@ -17,6 +19,33 @@
>   #include <efi.h>
>   #include <efi_api.h>
>
> +static struct efi_priv *global_priv;
> +
> +struct efi_priv *efi_get_priv(void)
> +{
> +	return global_priv;
> +}
> +
> +void efi_set_priv(struct efi_priv *priv)
> +{
> +	global_priv = priv;
> +}
> +
> +struct efi_system_table *efi_get_sys_table(void)
> +{
> +	return global_priv->sys_table;
> +}
> +
> +struct efi_boot_services *efi_get_boot(void)
> +{
> +	return global_priv->boot;
> +}
> +
> +unsigned long efi_get_ram_base(void)
> +{
> +	return global_priv->ram_base;
> +}
> +
>   /*
>    * Global declaration of gd.
>    *
> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> index 852cf3679d6..2f1feda1b1e 100644
> --- a/lib/efi/efi_app.c
> +++ b/lib/efi/efi_app.c
> @@ -27,23 +27,6 @@
>
>   DECLARE_GLOBAL_DATA_PTR;
>
> -static struct efi_priv *global_priv;
> -
> -struct efi_system_table *efi_get_sys_table(void)
> -{
> -	return global_priv->sys_table;
> -}
> -
> -struct efi_boot_services *efi_get_boot(void)
> -{
> -	return global_priv->boot;
> -}
> -
> -unsigned long efi_get_ram_base(void)
> -{
> -	return global_priv->ram_base;
> -}
> -
>   int efi_info_get(enum efi_entry_t type, void **datap, int *sizep)
>   {
>   	return -ENOSYS;
> @@ -318,7 +301,7 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>   	/* Set up access to EFI data structures */
>   	efi_init(priv, "App", image, sys_table);
>
> -	global_priv = priv;
> +	efi_set_priv(priv);
>
>   	/*
>   	 * Set up the EFI debug UART so that printf() works. This is
> @@ -344,7 +327,7 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>
>   static void efi_exit(void)
>   {
> -	struct efi_priv *priv = global_priv;
> +	struct efi_priv *priv = efi_get_priv();
>
>   	free_memory(priv);
>   	printf("U-Boot EFI exiting\n");
> diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c
> index 31f1e1a72a1..c89ae7c9072 100644
> --- a/lib/efi/efi_stub.c
> +++ b/lib/efi/efi_stub.c
> @@ -31,7 +31,6 @@
>   #error "This file needs to be ported for use on architectures"
>   #endif
>
> -static struct efi_priv *global_priv;
>   static bool use_uart;
>
>   struct __packed desctab_info {
> @@ -63,6 +62,8 @@ void _debug_uart_init(void)
>
>   void putc(const char ch)
>   {
> +	struct efi_priv *priv = efi_get_priv();
> +
>   	if (ch == '\n')
>   		putc('\r');
>
> @@ -73,7 +74,7 @@ void putc(const char ch)
>   			;
>   		outb(ch, (ulong)&com_port->thr);
>   	} else {
> -		efi_putc(global_priv, ch);
> +		efi_putc(priv, ch);
>   	}
>   }
>
> @@ -320,7 +321,7 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>   		puts(" efi_init() failed\n");
>   		return ret;
>   	}
> -	global_priv = priv;
> +	efi_set_priv(priv);
>
>   	cs32 = get_codeseg32();
>   	if (cs32 < 0)

Re: [PATCH v8 12/25] efi: Move exit_boot_services into a function

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> At present this code is inline in the app and stub. But they do the same
> thing. The difference is that the stub does it immediately and the app
> doesn't want to do it until the end (when it boots a kernel) or not at
> all, if returning to UEFI.
>
> Move it into a function so it can be called as needed.
>
> Also store the memory map so that it can be accessed within the app if
> needed.

The memory map is *not* a static object. It may change with any API call
that you make. You must read the memory map immediately before calling
ExitBootServices(). The valid value of MapKey typically will change with
any change of the memory map. Calling ExitBootServices() with the wrong
value of MapKey will lead to a failure. Storing these values except for
immediate use makes no sense.

>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v6)
>
> Changes in v6:
> - Fix typo in function comment
>
> Changes in v2:
> - Add a sentence about what the patch does
>
>   include/efi.h      | 32 ++++++++++++++++++++++
>   lib/efi/efi.c      | 68 ++++++++++++++++++++++++++++++++++++++++++++++
>   lib/efi/efi_app.c  |  3 ++
>   lib/efi/efi_stub.c | 66 ++++++++------------------------------------
>   4 files changed, 114 insertions(+), 55 deletions(-)
>
> diff --git a/include/efi.h b/include/efi.h
> index d4785478585..2a84223d235 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -407,6 +407,12 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
>    * @sys_table: Pointer to system table
>    * @boot: Pointer to boot-services table
>    * @run: Pointer to runtime-services table
> + * @memmap_key: Key returned from get_memory_map()
> + * @memmap_desc: List of memory-map records
> + * @memmap_alloc: Amount of memory allocated for memory map list
> + * @memmap_size Size of memory-map list in bytes
> + * @memmap_desc_size: Size of an individual memory-map record, in bytes
> + * @memmap_version: Memory-map version
>    *
>    * @use_pool_for_malloc: true if all allocation should go through the EFI 'pool'
>    *	methods allocate_pool() and free_pool(); false to use 'pages' methods
> @@ -424,6 +430,12 @@ struct efi_priv {
>   	struct efi_system_table *sys_table;
>   	struct efi_boot_services *boot;
>   	struct efi_runtime_services *run;
> +	efi_uintn_t memmap_key;
> +	struct efi_mem_desc *memmap_desc;
> +	efi_uintn_t memmap_alloc;
> +	efi_uintn_t memmap_size;
> +	efi_uintn_t memmap_desc_size;
> +	u32 memmap_version;
>
>   	/* app: */
>   	bool use_pool_for_malloc;
> @@ -574,4 +586,24 @@ void efi_putc(struct efi_priv *priv, const char ch);
>    */
>   int efi_info_get(enum efi_entry_t type, void **datap, int *sizep);
>
> +/**
> + * efi_store_memory_map() - Collect the memory-map info from EFI
> + *
> + * Collect the memory info and store it for later use, e.g. in calling
> + * exit_boot_services()
> + *
> + * @priv:	Pointer to private EFI structure
> + * @return 0 if OK, non-zero on error
> + */
> +int efi_store_memory_map(struct efi_priv *priv);
> +
> +/**
> + * efi_call_exit_boot_services() - Handle the exit-boot-service procedure
> + *
> + * Tell EFI we don't want their boot services anymore
> + *
> + * Return: 0 if OK, non-zero on error
> + */
> +int efi_call_exit_boot_services(void);
> +
>   #endif /* _LINUX_EFI_H */
> diff --git a/lib/efi/efi.c b/lib/efi/efi.c
> index cd6bf47b180..20da88c9151 100644
> --- a/lib/efi/efi.c
> +++ b/lib/efi/efi.c
> @@ -135,3 +135,71 @@ void efi_free(struct efi_priv *priv, void *ptr)
>
>   	boot->free_pool(ptr);
>   }
> +
> +int efi_store_memory_map(struct efi_priv *priv)
> +{
> +	struct efi_boot_services *boot = priv->sys_table->boottime;
> +	efi_uintn_t size, desc_size;
> +	efi_status_t ret;
> +
> +	/* Get the memory map so we can switch off EFI */
> +	size = 0;
> +	ret = boot->get_memory_map(&size, NULL, &priv->memmap_key,
> +				   &priv->memmap_desc_size,
> +				   &priv->memmap_version);
> +	if (ret != EFI_BUFFER_TOO_SMALL) {
> +		printhex2(EFI_BITS_PER_LONG);
> +		putc(' ');
> +		printhex2(ret);
> +		puts(" No memory map\n");
> +		return ret;
> +	}
> +	/*
> +	 * Since doing a malloc() may change the memory map and also we want to
> +	 * be able to read the memory map in efi_call_exit_boot_services()
> +	 * below, after more changes have happened
> +	 */
> +	priv->memmap_alloc = size + 1024;

GetMemoryMap() must be called immediately before calling ExitBootServices().

If efi_malloc() invokes AllocatePages() or AllocatePool(), you should
add DescriptorSize to MemoryMapSize and not some random value (e.g. 1024).

> +	priv->memmap_size = priv->memmap_alloc;
> +	priv->memmap_desc = efi_malloc(priv, size, &ret);
> +	if (!priv->memmap_desc) {
> +		printhex2(ret);
> +		puts(" No memory for memory descriptor\n");
> +		return ret;
> +	}
> +
> +	ret = boot->get_memory_map(&priv->memmap_size, priv->memmap_desc,
> +				   &priv->memmap_key, &desc_size,
> +				   &priv->memmap_version);
> +	if (ret) {
> +		printhex2(ret);
> +		puts(" Can't get memory map\n");
> +		return ret;

Please, use printf().

> +	}
> +
> +	return 0;
> +}
> +
> +int efi_call_exit_boot_services(void)
> +{
> +	struct efi_priv *priv = efi_get_priv();
> +	const struct efi_boot_services *boot = priv->boot;
> +	efi_uintn_t size;
> +	u32 version;
> +	efi_status_t ret;
> +
> +	size = priv->memmap_alloc;
> +	ret = boot->get_memory_map(&size, priv->memmap_desc,
> +				   &priv->memmap_key,
> +				   &priv->memmap_desc_size, &version);
> +	if (ret) {
> +		printhex2(ret);
> +		puts(" Can't get memory map\n");
> +		return ret;

Please, use printf().

Best regards

Heinrich

> +	}
> +	ret = boot->exit_boot_services(priv->parent_image, priv->memmap_key);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> index 2f1feda1b1e..8bd710d2e95 100644
> --- a/lib/efi/efi_app.c
> +++ b/lib/efi/efi_app.c
> @@ -315,6 +315,9 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>   		printf("Failed to set up memory: ret=%lx\n", ret);
>   		return ret;
>   	}
> +	ret = efi_store_memory_map(priv);
> +	if (ret)
> +		return ret;
>
>   	printf("starting\n");
>
> diff --git a/lib/efi/efi_stub.c b/lib/efi/efi_stub.c
> index c89ae7c9072..646cde3214c 100644
> --- a/lib/efi/efi_stub.c
> +++ b/lib/efi/efi_stub.c
> @@ -304,15 +304,12 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>   {
>   	struct efi_priv local_priv, *priv = &local_priv;
>   	struct efi_boot_services *boot = sys_table->boottime;
> -	struct efi_mem_desc *desc;
>   	struct efi_entry_memmap map;
>   	struct efi_gop *gop;
>   	struct efi_entry_gopmode mode;
>   	struct efi_entry_systable table;
>   	efi_guid_t efi_gop_guid = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
> -	efi_uintn_t key, desc_size, size;
>   	efi_status_t ret;
> -	u32 version;
>   	int cs32;
>
>   	ret = efi_init(priv, "Payload", image, sys_table);
> @@ -327,24 +324,11 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>   	if (cs32 < 0)
>   		return EFI_UNSUPPORTED;
>
> -	/* Get the memory map so we can switch off EFI */
> -	size = 0;
> -	ret = boot->get_memory_map(&size, NULL, &key, &desc_size, &version);
> -	if (ret != EFI_BUFFER_TOO_SMALL) {
> -		printhex2(EFI_BITS_PER_LONG);
> -		putc(' ');
> -		printhex2(ret);
> -		puts(" No memory map\n");
> -		return ret;
> -	}
> -	size += 1024;	/* Since doing a malloc() may change the memory map! */
> -	desc = efi_malloc(priv, size, &ret);
> -	if (!desc) {
> -		printhex2(ret);
> -		puts(" No memory for memory descriptor\n");
> +	ret = efi_store_memory_map(priv);
> +	if (ret)
>   		return ret;
> -	}
> -	ret = setup_info_table(priv, size + 128);
> +
> +	ret = setup_info_table(priv, priv->memmap_size + 128);
>   	if (ret)
>   		return ret;
>
> @@ -360,48 +344,20 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>   			       sizeof(struct efi_gop_mode_info));
>   	}
>
> -	ret = boot->get_memory_map(&size, desc, &key, &desc_size, &version);
> -	if (ret) {
> -		printhex2(ret);
> -		puts(" Can't get memory map\n");
> -		return ret;
> -	}
> -
>   	table.sys_table = (ulong)sys_table;
>   	add_entry_addr(priv, EFIET_SYS_TABLE, &table, sizeof(table), NULL, 0);
>
> -	ret = boot->exit_boot_services(image, key);
> -	if (ret) {
> -		/*
> -		 * Unfortunately it happens that we cannot exit boot services
> -		 * the first time. But the second time it work. I don't know
> -		 * why but this seems to be a repeatable problem. To get
> -		 * around it, just try again.
> -		 */
> -		printhex2(ret);
> -		puts(" Can't exit boot services\n");
> -		size = sizeof(desc);
> -		ret = boot->get_memory_map(&size, desc, &key, &desc_size,
> -					   &version);
> -		if (ret) {
> -			printhex2(ret);
> -			puts(" Can't get memory map\n");
> -			return ret;
> -		}
> -		ret = boot->exit_boot_services(image, key);
> -		if (ret) {
> -			printhex2(ret);
> -			puts(" Can't exit boot services 2\n");
> -			return ret;
> -		}
> -	}
> +	ret = efi_call_exit_boot_services();
> +	if (ret)
> +		return ret;
>
>   	/* The EFI UART won't work now, switch to a debug one */
>   	use_uart = true;
>
> -	map.version = version;
> -	map.desc_size = desc_size;
> -	add_entry_addr(priv, EFIET_MEMORY_MAP, &map, sizeof(map), desc, size);
> +	map.version = priv->memmap_version;
> +	map.desc_size = priv->memmap_desc_size;
> +	add_entry_addr(priv, EFIET_MEMORY_MAP, &map, sizeof(map),
> +		       priv->memmap_desc, priv->memmap_size);
>   	add_entry_addr(priv, EFIET_END, NULL, 0, 0, 0);
>
>   	memcpy((void *)CONFIG_SYS_TEXT_BASE, _binary_u_boot_bin_start,

Re: [PATCH v8 13/25] efi: Check for failure when initing the app

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> The stub checks for failure with efi_init(). Add this for the app as well.
> It is unlikely that anything can be done, but we may as well stop.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v6)
>
> Changes in v6:
> - Use 'U-Boot' instead of 'ARP' typo
>
>   lib/efi/efi_app.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> index 8bd710d2e95..30af414069d 100644
> --- a/lib/efi/efi_app.c
> +++ b/lib/efi/efi_app.c
> @@ -299,8 +299,11 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
>   	efi_status_t ret;
>
>   	/* Set up access to EFI data structures */
> -	efi_init(priv, "App", image, sys_table);
> -
> +	ret = efi_init(priv, "App", image, sys_table);

Please, document the return value of efi_init() in include/efi.h.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> +	if (ret) {
> +		printf("Failed to set up U-Boot: err=%lx\n", ret);
> +		return ret;
> +	}
>   	efi_set_priv(priv);
>
>   	/*

Re: [PATCH v8 14/25] efi: Mention that efi_info_get() is only used in the stub

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> This provides access to EFI tables after U-Boot has exited boot services.
> It is not needed in the app since boot services remain alive and we can
> just call them whenever needed.
>
> Add a comment to explain this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v6)
>
> Changes in v6:
> - Fix 'stuff' typo in comment
> - Update to 'This function' in comment
>
> Changes in v2:
> - Fix 'as' typo
>
>   include/efi.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/efi.h b/include/efi.h
> index 2a84223d235..dbf4ffaaf02 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -578,6 +578,10 @@ void efi_putc(struct efi_priv *priv, const char ch);
>   /**
>    * efi_info_get() - get an entry from an EFI table
>    *
> + * This function is called from U-Boot proper to read information set up by the
> + * EFI stub. It can only be used when running from the EFI stub, not when U-Boot
> + * is running as an app.
> + *

Please have a look at the rest of the description:

>    * @type:	Entry type to search for
>    * @datap:	Returns pointer to entry data
>    * @sizep:	Returns pointer to entry size

An integer is returned, not a pointer.
Please, use Return: instead of @return:

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Re: [PATCH v8 15/25] efi: Show when allocated pages are used

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> Add a message here so that both paths of memory allocation are reported.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Re: [PATCH v8 16/25] efi: Allow easy selection of serial-only operation

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> Add info about how to select vidconsole or serial.
>
> Also set up a demo boot command.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Add a better boot command too
>
>   include/configs/efi-x86_app.h | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/include/configs/efi-x86_app.h b/include/configs/efi-x86_app.h
> index 6061a6db0a4..33afb7ca0f9 100644
> --- a/include/configs/efi-x86_app.h
> +++ b/include/configs/efi-x86_app.h
> @@ -10,8 +10,33 @@
>
>   #undef CONFIG_TPM_TIS_BASE_ADDRESS
>
> +/*
> + * Select the output device: Put an 'x' prefix before one of these to disable it
> + */
> +
> +/*
> + * Video output - can normally continue after exit_boot_services has been
> + * called, since output to the display does not require EFI services at that
> + * point. U-Boot sets up the console memory and does its own drawing.
> + */
>   #define CONFIG_STD_DEVICES_SETTINGS	"stdin=serial\0" \
>   					"stdout=vidconsole\0" \
>   					"stderr=vidconsole\0"
>
> +/*
> + * Serial output with no console. Run qemu with:
> + *
> + *    -display none -serial mon:stdio
> + *
> + * This will hang or fail to output on the console after exit_boot_services is
> + * called.

In lib/efi/efi_stub.c, efi_main() you set use_uart=true. Couldn't you do
a similar thing in the app?

Instead of hard coding a specific UART it would be preferable both for
the EFI stub as well as for the app to use the debug console defined by
CONFIG_DEBUG_UART.

Best regards

Heinrich

> + */
> +#define xCONFIG_STD_DEVICES_SETTINGS	"stdin=serial\0" \
> +					"stdout=serial\0" \
> +					"stderr=serial\0"
> +
> +#undef CONFIG_BOOTCOMMAND
> +
> +#define CONFIG_BOOTCOMMAND "part list efi 0; fatls efi 0:1"
> +
>   #endif

Re: [PATCH v8 19/25] x86: efi: Show the system-table revision

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> Show the revision of this table as it can be important.
>
> Also update the 'efi table' entry to show the actual address of the EFI
> table rather than our table that points to it. This saves a step and the
> intermediate table has nothing else in it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v6)
>
> Changes in v6:
> - Fix Alo typo in commit message
>
> Changes in v5:
> - Fix grammar in commit message
>
> Changes in v3:
> - Add new patch to show the system-table revision
>
>   arch/x86/cpu/efi/payload.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/cpu/efi/payload.c b/arch/x86/cpu/efi/payload.c
> index d2aa889a2b9..b7778565b19 100644
> --- a/arch/x86/cpu/efi/payload.c
> +++ b/arch/x86/cpu/efi/payload.c
> @@ -7,6 +7,7 @@
>   #include <common.h>
>   #include <cpu_func.h>
>   #include <efi.h>
> +#include <efi_api.h>
>   #include <errno.h>
>   #include <init.h>
>   #include <log.h>
> @@ -296,8 +297,14 @@ void setup_efi_info(struct efi_info *efi_info)
>   void efi_show_bdinfo(void)
>   {
>   	struct efi_entry_systable *table = NULL;
> +	struct efi_system_table *sys_table;
>   	int size, ret;
>
>   	ret = efi_info_get(EFIET_SYS_TABLE, (void **)&table, &size);
> -	bdinfo_print_num_l("efi_table", (ulong)table);
> +	if (!ret) {
> +		bdinfo_print_num_l("efi_table", table->sys_table);
> +		sys_table = (struct efi_system_table *)(uintptr_t)
> +			table->sys_table;
> +		bdinfo_print_num_l(" revision", sys_table->fw_revision);

This will print "0x0000025A" for UEFI version 2.9.
Should we print "2.9" instead?

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> +	}
>   }

Re: [PATCH v8 20/25] x86: efi: Don't set up global_data again with EFI

[Heinrich Schuchardt]

On 12/29/21 19:57, Simon Glass wrote:
> Since EFI does not relocate and uses the same global_data pointer
> throughout the board-init process, drop this unnecessary setup, to avoid
> a hang.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Re: [PATCH v8 12/25] efi: Move exit_boot_services into a function

[Simon Glass]

Hi Heinrich,

On Thu, 30 Dec 2021 at 22:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12/29/21 19:57, Simon Glass wrote:
> > At present this code is inline in the app and stub. But they do the same
> > thing. The difference is that the stub does it immediately and the app
> > doesn't want to do it until the end (when it boots a kernel) or not at
> > all, if returning to UEFI.
> >
> > Move it into a function so it can be called as needed.
> >
> > Also store the memory map so that it can be accessed within the app if
> > needed.
>
> The memory map is *not* a static object. It may change with any API call
> that you make. You must read the memory map immediately before calling
> ExitBootServices(). The valid value of MapKey typically will change with
> any change of the memory map. Calling ExitBootServices() with the wrong
> value of MapKey will lead to a failure. Storing these values except for
> immediate use makes no sense.
>
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v6)
> >
> > Changes in v6:
> > - Fix typo in function comment
> >
> > Changes in v2:
> > - Add a sentence about what the patch does
> >
> >   include/efi.h      | 32 ++++++++++++++++++++++
> >   lib/efi/efi.c      | 68 ++++++++++++++++++++++++++++++++++++++++++++++
> >   lib/efi/efi_app.c  |  3 ++
> >   lib/efi/efi_stub.c | 66 ++++++++------------------------------------
> >   4 files changed, 114 insertions(+), 55 deletions(-)
> >
> > diff --git a/include/efi.h b/include/efi.h
> > index d4785478585..2a84223d235 100644
> > --- a/include/efi.h
> > +++ b/include/efi.h
> > @@ -407,6 +407,12 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
> >    * @sys_table: Pointer to system table
> >    * @boot: Pointer to boot-services table
> >    * @run: Pointer to runtime-services table
> > + * @memmap_key: Key returned from get_memory_map()
> > + * @memmap_desc: List of memory-map records
> > + * @memmap_alloc: Amount of memory allocated for memory map list
> > + * @memmap_size Size of memory-map list in bytes
> > + * @memmap_desc_size: Size of an individual memory-map record, in bytes
> > + * @memmap_version: Memory-map version
> >    *
> >    * @use_pool_for_malloc: true if all allocation should go through the EFI 'pool'
> >    *  methods allocate_pool() and free_pool(); false to use 'pages' methods
> > @@ -424,6 +430,12 @@ struct efi_priv {
> >       struct efi_system_table *sys_table;
> >       struct efi_boot_services *boot;
> >       struct efi_runtime_services *run;
> > +     efi_uintn_t memmap_key;
> > +     struct efi_mem_desc *memmap_desc;
> > +     efi_uintn_t memmap_alloc;
> > +     efi_uintn_t memmap_size;
> > +     efi_uintn_t memmap_desc_size;
> > +     u32 memmap_version;
> >
> >       /* app: */
> >       bool use_pool_for_malloc;
> > @@ -574,4 +586,24 @@ void efi_putc(struct efi_priv *priv, const char ch);
> >    */
> >   int efi_info_get(enum efi_entry_t type, void **datap, int *sizep);
> >
> > +/**
> > + * efi_store_memory_map() - Collect the memory-map info from EFI
> > + *
> > + * Collect the memory info and store it for later use, e.g. in calling
> > + * exit_boot_services()
> > + *
> > + * @priv:    Pointer to private EFI structure
> > + * @return 0 if OK, non-zero on error
> > + */
> > +int efi_store_memory_map(struct efi_priv *priv);
> > +
> > +/**
> > + * efi_call_exit_boot_services() - Handle the exit-boot-service procedure
> > + *
> > + * Tell EFI we don't want their boot services anymore
> > + *
> > + * Return: 0 if OK, non-zero on error
> > + */
> > +int efi_call_exit_boot_services(void);
> > +
> >   #endif /* _LINUX_EFI_H */
> > diff --git a/lib/efi/efi.c b/lib/efi/efi.c
> > index cd6bf47b180..20da88c9151 100644
> > --- a/lib/efi/efi.c
> > +++ b/lib/efi/efi.c
> > @@ -135,3 +135,71 @@ void efi_free(struct efi_priv *priv, void *ptr)
> >
> >       boot->free_pool(ptr);
> >   }
> > +
> > +int efi_store_memory_map(struct efi_priv *priv)
> > +{
> > +     struct efi_boot_services *boot = priv->sys_table->boottime;
> > +     efi_uintn_t size, desc_size;
> > +     efi_status_t ret;
> > +
> > +     /* Get the memory map so we can switch off EFI */
> > +     size = 0;
> > +     ret = boot->get_memory_map(&size, NULL, &priv->memmap_key,
> > +                                &priv->memmap_desc_size,
> > +                                &priv->memmap_version);
> > +     if (ret != EFI_BUFFER_TOO_SMALL) {
> > +             printhex2(EFI_BITS_PER_LONG);
> > +             putc(' ');
> > +             printhex2(ret);
> > +             puts(" No memory map\n");
> > +             return ret;
> > +     }
> > +     /*
> > +      * Since doing a malloc() may change the memory map and also we want to
> > +      * be able to read the memory map in efi_call_exit_boot_services()
> > +      * below, after more changes have happened
> > +      */
> > +     priv->memmap_alloc = size + 1024;
>
> GetMemoryMap() must be called immediately before calling ExitBootServices().

Yes that's right. I remember reading that in the spec.

>
> If efi_malloc() invokes AllocatePages() or AllocatePohl(), you should
> add DescriptorSize to MemoryMapSize and not some random value (e.g. 1024).

This is copying existing code. If you want to change this, we can do
it in a follow-on patch.

For that discussion, How do you know it will be increased by
sizeof(DescriptorSize) ? Is that in the spec somewhere?

In one run of the stub with qemu it returned 0x1170 for the first call
and only 0x1050 for the second. In a run of the app, I got 11d0 and
then 10b0.

Is it possible that some EFI implementations allow a margin already?

Otherwise, what do you suggest?

>
> > +     priv->memmap_size = priv->memmap_alloc;
> > +     priv->memmap_desc = efi_malloc(priv, size, &ret);
> > +     if (!priv->memmap_desc) {
> > +             printhex2(ret);
> > +             puts(" No memory for memory descriptor\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = boot->get_memory_map(&priv->memmap_size, priv->memmap_desc,
> > +                                &priv->memmap_key, &desc_size,
> > +                                &priv->memmap_version);
> > +     if (ret) {
> > +             printhex2(ret);
> > +             puts(" Can't get memory map\n");
> > +             return ret;
>
> Please, use printf().

It is not available in the stub. Even puts() is only available because
there is a local version in efi_stub.c. I'll add a comment.

>
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +int efi_call_exit_boot_services(void)
> > +{
> > +     struct efi_priv *priv = efi_get_priv();
> > +     const struct efi_boot_services *boot = priv->boot;
> > +     efi_uintn_t size;
> > +     u32 version;
> > +     efi_status_t ret;
> > +
> > +     size = priv->memmap_alloc;
> > +     ret = boot->get_memory_map(&size, priv->memmap_desc,
> > +                                &priv->memmap_key,
> > +                                &priv->memmap_desc_size, &version);
> > +     if (ret) {
> > +             printhex2(ret);
> > +             puts(" Can't get memory map\n");
> > +             return ret;
>
> Please, use printf().

See above.

Regards,
Simon

Re: [PATCH v8 16/25] efi: Allow easy selection of serial-only operation

[Simon Glass]

Hi Heinrich,

On Thu, 30 Dec 2021 at 23:18, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12/29/21 19:57, Simon Glass wrote:
> > Add info about how to select vidconsole or serial.
> >
> > Also set up a demo boot command.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v2)
> >
> > Changes in v2:
> > - Add a better boot command too
> >
> >   include/configs/efi-x86_app.h | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> >
> > diff --git a/include/configs/efi-x86_app.h b/include/configs/efi-x86_app.h
> > index 6061a6db0a4..33afb7ca0f9 100644
> > --- a/include/configs/efi-x86_app.h
> > +++ b/include/configs/efi-x86_app.h
> > @@ -10,8 +10,33 @@
> >
> >   #undef CONFIG_TPM_TIS_BASE_ADDRESS
> >
> > +/*
> > + * Select the output device: Put an 'x' prefix before one of these to disable it
> > + */
> > +
> > +/*
> > + * Video output - can normally continue after exit_boot_services has been
> > + * called, since output to the display does not require EFI services at that
> > + * point. U-Boot sets up the console memory and does its own drawing.
> > + */
> >   #define CONFIG_STD_DEVICES_SETTINGS "stdin=serial\0" \
> >                                       "stdout=vidconsole\0" \
> >                                       "stderr=vidconsole\0"
> >
> > +/*
> > + * Serial output with no console. Run qemu with:
> > + *
> > + *    -display none -serial mon:stdio
> > + *
> > + * This will hang or fail to output on the console after exit_boot_services is
> > + * called.
>
> In lib/efi/efi_stub.c, efi_main() you set use_uart=true. Couldn't you do
> a similar thing in the app?

Well use_uart is a bit of a hack - just using a hard-coded x86 UART.
See putc() in efi_stub.c - it is just for debugging. There seem to be
importants periods in EFI where debug output is not supported.

>
> Instead of hard coding a specific UART it would be preferable both for
> the EFI stub as well as for the app to use the debug console defined by
> CONFIG_DEBUG_UART.

Well the app uses the con_out thing, so doesn't use a specific UART.

The stub uses the same thing while EFI is available, but uses the hack
when it is not. The console used by DEBUG_UART should be the same
driver as the 'serial' one selected here, so I think it is already
what you want.

I suspect we can make the serial and video enable/disable at runtime
but that seems like a separate problem.

Regards,
Simon

Re: [PATCH v8 19/25] x86: efi: Show the system-table revision

[Simon Glass]

Hi Heinrich,

On Thu, 30 Dec 2021 at 23:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12/29/21 19:57, Simon Glass wrote:
> > Show the revision of this table as it can be important.
> >
> > Also update the 'efi table' entry to show the actual address of the EFI
> > table rather than our table that points to it. This saves a step and the
> > intermediate table has nothing else in it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (no changes since v6)
> >
> > Changes in v6:
> > - Fix Alo typo in commit message
> >
> > Changes in v5:
> > - Fix grammar in commit message
> >
> > Changes in v3:
> > - Add new patch to show the system-table revision
> >
> >   arch/x86/cpu/efi/payload.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/cpu/efi/payload.c b/arch/x86/cpu/efi/payload.c
> > index d2aa889a2b9..b7778565b19 100644
> > --- a/arch/x86/cpu/efi/payload.c
> > +++ b/arch/x86/cpu/efi/payload.c
> > @@ -7,6 +7,7 @@
> >   #include <common.h>
> >   #include <cpu_func.h>
> >   #include <efi.h>
> > +#include <efi_api.h>
> >   #include <errno.h>
> >   #include <init.h>
> >   #include <log.h>
> > @@ -296,8 +297,14 @@ void setup_efi_info(struct efi_info *efi_info)
> >   void efi_show_bdinfo(void)
> >   {
> >       struct efi_entry_systable *table = NULL;
> > +     struct efi_system_table *sys_table;
> >       int size, ret;
> >
> >       ret = efi_info_get(EFIET_SYS_TABLE, (void **)&table, &size);
> > -     bdinfo_print_num_l("efi_table", (ulong)table);
> > +     if (!ret) {
> > +             bdinfo_print_num_l("efi_table", table->sys_table);
> > +             sys_table = (struct efi_system_table *)(uintptr_t)
> > +                     table->sys_table;
> > +             bdinfo_print_num_l(" revision", sys_table->fw_revision);
>
> This will print "0x0000025A" for UEFI version 2.9.
> Should we print "2.9" instead?
>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

We coud...

The spec says (in it's strange hyphen-free style):

FirmwareRevision A firmware vendor specific value that identifies the
revision of the
system firmware for the platform.

How do you know the format of the value? For the OVMF version I see 0x00010000

Regards,
Simon

Re: [PATCH v8 12/25] efi: Move exit_boot_services into a function

[Heinrich Schuchardt]

On 1/4/22 11:52, Simon Glass wrote:
> Hi Heinrich,
>
> On Thu, 30 Dec 2021 at 22:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 12/29/21 19:57, Simon Glass wrote:
>>> At present this code is inline in the app and stub. But they do the same
>>> thing. The difference is that the stub does it immediately and the app
>>> doesn't want to do it until the end (when it boots a kernel) or not at
>>> all, if returning to UEFI.
>>>
>>> Move it into a function so it can be called as needed.
>>>
>>> Also store the memory map so that it can be accessed within the app if
>>> needed.
>>
>> The memory map is *not* a static object. It may change with any API call
>> that you make. You must read the memory map immediately before calling
>> ExitBootServices(). The valid value of MapKey typically will change with
>> any change of the memory map. Calling ExitBootServices() with the wrong
>> value of MapKey will lead to a failure. Storing these values except for
>> immediate use makes no sense.
>>
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> (no changes since v6)
>>>
>>> Changes in v6:
>>> - Fix typo in function comment
>>>
>>> Changes in v2:
>>> - Add a sentence about what the patch does
>>>
>>>    include/efi.h      | 32 ++++++++++++++++++++++
>>>    lib/efi/efi.c      | 68 ++++++++++++++++++++++++++++++++++++++++++++++
>>>    lib/efi/efi_app.c  |  3 ++
>>>    lib/efi/efi_stub.c | 66 ++++++++------------------------------------
>>>    4 files changed, 114 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/include/efi.h b/include/efi.h
>>> index d4785478585..2a84223d235 100644
>>> --- a/include/efi.h
>>> +++ b/include/efi.h
>>> @@ -407,6 +407,12 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
>>>     * @sys_table: Pointer to system table
>>>     * @boot: Pointer to boot-services table
>>>     * @run: Pointer to runtime-services table
>>> + * @memmap_key: Key returned from get_memory_map()
>>> + * @memmap_desc: List of memory-map records
>>> + * @memmap_alloc: Amount of memory allocated for memory map list
>>> + * @memmap_size Size of memory-map list in bytes
>>> + * @memmap_desc_size: Size of an individual memory-map record, in bytes
>>> + * @memmap_version: Memory-map version
>>>     *
>>>     * @use_pool_for_malloc: true if all allocation should go through the EFI 'pool'
>>>     *  methods allocate_pool() and free_pool(); false to use 'pages' methods
>>> @@ -424,6 +430,12 @@ struct efi_priv {
>>>        struct efi_system_table *sys_table;
>>>        struct efi_boot_services *boot;
>>>        struct efi_runtime_services *run;
>>> +     efi_uintn_t memmap_key;
>>> +     struct efi_mem_desc *memmap_desc;
>>> +     efi_uintn_t memmap_alloc;
>>> +     efi_uintn_t memmap_size;
>>> +     efi_uintn_t memmap_desc_size;
>>> +     u32 memmap_version;
>>>
>>>        /* app: */
>>>        bool use_pool_for_malloc;
>>> @@ -574,4 +586,24 @@ void efi_putc(struct efi_priv *priv, const char ch);
>>>     */
>>>    int efi_info_get(enum efi_entry_t type, void **datap, int *sizep);
>>>
>>> +/**
>>> + * efi_store_memory_map() - Collect the memory-map info from EFI
>>> + *
>>> + * Collect the memory info and store it for later use, e.g. in calling
>>> + * exit_boot_services()
>>> + *
>>> + * @priv:    Pointer to private EFI structure
>>> + * @return 0 if OK, non-zero on error
>>> + */
>>> +int efi_store_memory_map(struct efi_priv *priv);
>>> +
>>> +/**
>>> + * efi_call_exit_boot_services() - Handle the exit-boot-service procedure
>>> + *
>>> + * Tell EFI we don't want their boot services anymore
>>> + *
>>> + * Return: 0 if OK, non-zero on error
>>> + */
>>> +int efi_call_exit_boot_services(void);
>>> +
>>>    #endif /* _LINUX_EFI_H */
>>> diff --git a/lib/efi/efi.c b/lib/efi/efi.c
>>> index cd6bf47b180..20da88c9151 100644
>>> --- a/lib/efi/efi.c
>>> +++ b/lib/efi/efi.c
>>> @@ -135,3 +135,71 @@ void efi_free(struct efi_priv *priv, void *ptr)
>>>
>>>        boot->free_pool(ptr);
>>>    }
>>> +
>>> +int efi_store_memory_map(struct efi_priv *priv)
>>> +{
>>> +     struct efi_boot_services *boot = priv->sys_table->boottime;
>>> +     efi_uintn_t size, desc_size;
>>> +     efi_status_t ret;
>>> +
>>> +     /* Get the memory map so we can switch off EFI */
>>> +     size = 0;
>>> +     ret = boot->get_memory_map(&size, NULL, &priv->memmap_key,
>>> +                                &priv->memmap_desc_size,
>>> +                                &priv->memmap_version);
>>> +     if (ret != EFI_BUFFER_TOO_SMALL) {
>>> +             printhex2(EFI_BITS_PER_LONG);
>>> +             putc(' ');
>>> +             printhex2(ret);
>>> +             puts(" No memory map\n");
>>> +             return ret;
>>> +     }
>>> +     /*
>>> +      * Since doing a malloc() may change the memory map and also we want to
>>> +      * be able to read the memory map in efi_call_exit_boot_services()
>>> +      * below, after more changes have happened
>>> +      */
>>> +     priv->memmap_alloc = size + 1024;
>>
>> GetMemoryMap() must be called immediately before calling ExitBootServices().
>
> Yes that's right. I remember reading that in the spec.
>
>>
>> If efi_malloc() invokes AllocatePages() or AllocatePohl(), you should
>> add DescriptorSize to MemoryMapSize and not some random value (e.g. 1024).
>
> This is copying existing code. If you want to change this, we can do
> it in a follow-on patch.
>
> For that discussion, How do you know it will be increased by
> sizeof(DescriptorSize) ? Is that in the spec somewhere?
>
> In one run of the stub with qemu it returned 0x1170 for the first call
> and only 0x1050 for the second. In a run of the app, I got 11d0 and
> then 10b0.
>
> Is it possible that some EFI implementations allow a margin already?
>
> Otherwise, what do you suggest?

The UEFI 2.9 spec states:

"The actual size of the buffer allocated for the consequent call to
GetMemoryMap() should be bigger then the value returned in
MemoryMapSize, since allocation of the new buffer may potentially
increase memory map size."

The additional size needed will be a multiple of DescriptorSize. In
U-Boot carving out a memory area will create exactly create one extra
descriptor. But the UEFI spec gives no guarantee for this.

EDK II uses the following algorithm in multiple places.

MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c:147
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c:788
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c:163
MdePkg/Library/SmmMemLib/SmmMemLib.c:485

   Status = gBS->GetMemoryMap (
                   &MemoryMapSize,
                   MemoryMap,
                   &MapKey,
                   &DescriptorSize,
                   &DescriptorVersion
                   );
   ASSERT (Status == EFI_BUFFER_TOO_SMALL);
   do {
     MemoryMap = (EFI_MEMORY_DESCRIPTOR *) AllocatePool (MemoryMapSize);
     ASSERT (MemoryMap != NULL);
     Status = gBS->GetMemoryMap (
                     &MemoryMapSize,
                     MemoryMap,
                     &MapKey,
                     &DescriptorSize,
                     &DescriptorVersion
                     );
     if (EFI_ERROR (Status)) {
       FreePool (MemoryMap);
     }
   } while (Status == EFI_BUFFER_TOO_SMALL);
   ASSERT_EFI_ERROR (Status);

This algorithm will only work if FreePool() reverts the memory map
changes of AllocatePool() which might not be the case for simplistic
implementations.

In the UEFI SCT I found:

uefi-sct/SctPkg/SCRT/SCRTApp/SCRTApp.c-307-
	MemoryMapSize += 1024;

uefi-sct/SctPkg/TestCase/UEFI/EFI/BootServices/MemoryAllocationServices/BlackBoxTest/MemoryAllocationServicesBBTestMain.c-248
	MemoryMapSizeNeeded += EFI_PAGE_SIZE;

So let's keep your code as it is.

Best regards

Heinrich

>
>>
>>> +     priv->memmap_size = priv->memmap_alloc;
>>> +     priv->memmap_desc = efi_malloc(priv, size, &ret);
>>> +     if (!priv->memmap_desc) {
>>> +             printhex2(ret);
>>> +             puts(" No memory for memory descriptor\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = boot->get_memory_map(&priv->memmap_size, priv->memmap_desc,
>>> +                                &priv->memmap_key, &desc_size,
>>> +                                &priv->memmap_version);
>>> +     if (ret) {
>>> +             printhex2(ret);
>>> +             puts(" Can't get memory map\n");
>>> +             return ret;
>>
>> Please, use printf().
>
> It is not available in the stub. Even puts() is only available because
> there is a local version in efi_stub.c. I'll add a comment.
>
>>
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +int efi_call_exit_boot_services(void)
>>> +{
>>> +     struct efi_priv *priv = efi_get_priv();
>>> +     const struct efi_boot_services *boot = priv->boot;
>>> +     efi_uintn_t size;
>>> +     u32 version;
>>> +     efi_status_t ret;
>>> +
>>> +     size = priv->memmap_alloc;
>>> +     ret = boot->get_memory_map(&size, priv->memmap_desc,
>>> +                                &priv->memmap_key,
>>> +                                &priv->memmap_desc_size, &version);
>>> +     if (ret) {
>>> +             printhex2(ret);
>>> +             puts(" Can't get memory map\n");
>>> +             return ret;
>>
>> Please, use printf().
>
> See above.
>
> Regards,
> Simon