[U-Boot] [PATCH v2 00/16] efi: Enable basic sandbox support for EFI loader

[U-Boot] [PATCH v2 00/16] efi: Enable basic sandbox support for EFI loader

[Simon Glass]

A limitation of the EFI loader at present is that it does not build with
sandbox. This makes it hard to write tests, since sandbox is used for most
testing in U-Boot.

This series enables the EFI loader feature. It allows sandbox to build and
run a trivial function which calls the EFI API to output a message.

Much work remains but this should serve as a basis for adding tests more
easily for EFI loader.

This series sits on top of Heinrich's recent EFI test series. It is
available at u-boot-dm/efi-working

Changes in v2:
- Update return type of efi_smbios_register() to efi_status_t
- Use return value of efi_install_configuration_table
- Change return type of efi_init_obj_list() to efi_status_t
- Update commit message to dropping libfdt_env.h
- Update to use mapmem instead of a cast
- Rebase to master

Simon Glass (16):
  efi: Update efi_smbios_register() to return error code
  efi: Move the init check inside efi_init_obj_list()
  efi: Add error checking for efi_init_obj_list()
  efi: Add a TODO to efi_init_obj_list()
  efi: Correct header order in efi_memory
  efi: sandbox: Adjust memory usage for sandbox
  sandbox: smbios: Update to support sandbox
  sandbox: Add a setjmp() implementation
  efi: sandbox: Add required linker sections
  efi: sandbox: Add distroboot support
  Define board_quiesce_devices() in a shared location
  Add a comment for board_quiesce_devices()
  efi: sandbox: Add relocation constants
  efi: Add a comment about duplicated ELF constants
  efi: sandbox: Enable EFI loader builder for sandbox
  efi: sandbox: Add a simple 'bootefi test' command

 arch/arm/include/asm/u-boot-arm.h |  1 -
 arch/sandbox/cpu/cpu.c            | 13 ++++++++
 arch/sandbox/cpu/os.c             | 17 +++++++++++
 arch/sandbox/cpu/u-boot.lds       | 29 ++++++++++++++++++
 arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++
 arch/sandbox/lib/Makefile         |  2 +-
 arch/sandbox/lib/sections.c       | 12 ++++++++
 arch/x86/include/asm/u-boot-x86.h |  1 -
 arch/x86/lib/bootm.c              |  4 ---
 cmd/bootefi.c                     | 62 +++++++++++++++++++++++++++++++++++----
 common/bootm.c                    |  4 +++
 configs/sandbox_defconfig         |  1 +
 include/bootm.h                   |  8 +++++
 include/config_distro_bootcmd.h   |  2 +-
 include/efi_loader.h              | 12 +++++++-
 include/os.h                      | 21 +++++++++++++
 lib/efi_loader/Kconfig            | 12 +++++++-
 lib/efi_loader/Makefile           |  1 +
 lib/efi_loader/efi_memory.c       | 36 +++++++++++++----------
 lib/efi_loader/efi_runtime.c      |  7 +++++
 lib/efi_loader/efi_smbios.c       |  7 +++--
 lib/efi_loader/efi_test.c         | 17 +++++++++++
 lib/smbios.c                      | 38 ++++++++++++++++++------
 23 files changed, 284 insertions(+), 44 deletions(-)
 create mode 100644 arch/sandbox/include/asm/setjmp.h
 create mode 100644 arch/sandbox/lib/sections.c
 create mode 100644 lib/efi_loader/efi_test.c

-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 01/16] efi: Update efi_smbios_register() to return error code

[Simon Glass]

This function can fail but gives no indication of failure. Update it to
return an error when something goes wrong.

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

Changes in v2:
- Update return type of efi_smbios_register() to efi_status_t
- Use return value of efi_install_configuration_table

 include/efi_loader.h        | 9 ++++++++-
 lib/efi_loader/efi_smbios.c | 7 ++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 1b92edbd77..35f8f84401 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -164,7 +164,14 @@ int efi_gop_register(void);
 /* Called by bootefi to make the network interface available */
 int efi_net_register(void);
 /* Called by bootefi to make SMBIOS tables available */
-void efi_smbios_register(void);
+/**
+ * efi_smbios_register() - write out SMBIOS tables
+ *
+ * Called by bootefi to make SMBIOS tables available
+ *
+ * @return 0 if OK, -ENOMEM if no memory is available for the tables
+ */
+efi_status_t efi_smbios_register(void);
 
 struct efi_simple_file_system_protocol *
 efi_fs_from_path(struct efi_device_path *fp);
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index ac412e7362..67f71892ca 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -13,7 +13,7 @@
 
 static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
 
-void efi_smbios_register(void)
+efi_status_t efi_smbios_register(void)
 {
 	/* Map within the low 32 bits, to allow for 32bit SMBIOS tables */
 	uint64_t dmi = 0xffffffff;
@@ -22,11 +22,12 @@ void efi_smbios_register(void)
 	int memtype = EFI_RUNTIME_SERVICES_DATA;
 
 	if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS)
-		return;
+		return EFI_OUT_OF_RESOURCES;
 
 	/* Generate SMBIOS tables */
 	write_smbios_table(dmi);
 
 	/* And expose them to our EFI payload */
-	efi_install_configuration_table(&smbios_guid, (void*)(uintptr_t)dmi);
+	return efi_install_configuration_table(&smbios_guid,
+					       (void *)(uintptr_t)dmi);
 }
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 02/16] efi: Move the init check inside efi_init_obj_list()

[Simon Glass]

Rather than having the caller check this variable and the callee set it,
move all access to the variable inside the function. This reduces the
logic needed to call efi_init_obj_list().

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

Changes in v2: None

 cmd/bootefi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 478bc116e2..17b26e6f4e 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -28,6 +28,8 @@ static struct efi_device_path *bootefi_device_path;
 /* Initialize and populate EFI object list */
 static void efi_init_obj_list(void)
 {
+	if (efi_obj_list_initalized)
+		return;
 	efi_obj_list_initalized = 1;
 
 	efi_console_register();
@@ -208,6 +210,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
 	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
 		"{ro,boot}(blob)0000000000000000");
 
+	/* Initialize and populate EFI object list */
+	efi_init_obj_list();
+
 	/* Call our payload! */
 	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
 
@@ -310,6 +315,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		/* Initialize and populate EFI object list */
 		if (!efi_obj_list_initalized)
 			efi_init_obj_list();
+		loaded_image_info.device_handle = bootefi_device_path;
+		loaded_image_info.file_path = bootefi_image_path;
 		return efi_selftest(&loaded_image_info, &systab);
 	} else
 #endif
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 03/16] efi: Add error checking for efi_init_obj_list()

[Simon Glass]

This function calls a function which can fail. Print a message in this
case and abort the boot, rather than silently continuing to boot, which
will certainly fail.

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

Changes in v2:
- Change return type of efi_init_obj_list() to efi_status_t

 cmd/bootefi.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 17b26e6f4e..a2138f6075 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized;
 static struct efi_device_path *bootefi_image_path;
 static struct efi_device_path *bootefi_device_path;
 
-/* Initialize and populate EFI object list */
-static void efi_init_obj_list(void)
+/**
+ * efi_init_obj_list() - Initialize and populate EFI object list
+ *
+ * @return 0 if OK, -ve on error (in which case it prints a message)
+ */
+static efi_status_t efi_init_obj_list(void)
 {
+	efi_status_t ret;
+
 	if (efi_obj_list_initalized)
-		return;
+		return 0;
 	efi_obj_list_initalized = 1;
 
 	efi_console_register();
@@ -43,12 +49,19 @@ static void efi_init_obj_list(void)
 	efi_net_register();
 #endif
 #ifdef CONFIG_GENERATE_SMBIOS_TABLE
-	efi_smbios_register();
+	ret = efi_smbios_register();
+	if (ret)
+		goto error;
 #endif
 
 	/* Initialize EFI runtime services */
 	efi_reset_system_init();
 	efi_get_time_init();
+
+	return EFI_SUCCESS;
+error:
+	printf("Error: Cannot set up EFI object list (err=%d)\n", ret);
+	return ret;
 }
 
 static void *copy_fdt(void *fdt)
@@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
 	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
 	const efi_guid_t fdt_guid = EFI_FDT_GUID;
 	bootm_headers_t img = { 0 };
+	int ret;
 
 	/*
 	 * Special case for efi payload not loaded from disk, such as
@@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
 		"{ro,boot}(blob)0000000000000000");
 
 	/* Initialize and populate EFI object list */
-	efi_init_obj_list();
+	ret = efi_init_obj_list();
+	if (ret)
+		return ret;
 
 	/* Call our payload! */
 	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
@@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		 */
 		efi_save_gd();
 		/* Initialize and populate EFI object list */
-		if (!efi_obj_list_initalized)
-			efi_init_obj_list();
+		if (!efi_obj_list_initalized && efi_init_obj_list())
+			return CMD_RET_FAILURE;
+
 		loaded_image_info.device_handle = bootefi_device_path;
 		loaded_image_info.file_path = bootefi_image_path;
+
 		return efi_selftest(&loaded_image_info, &systab);
 	} else
 #endif
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 04/16] efi: Add a TODO to efi_init_obj_list()

[Simon Glass]

This function repeats data structures provided by driver model. They are
only created once so can be stale if the EFI loader is called twice (e.g.
for testing or on boot failure).

Add a TODO to address this. It should be possible to attach EFI devices
and data structures to driver-model devices and avoid having a parallel
set of data structures.

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

Changes in v2: None

 cmd/bootefi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index a2138f6075..a4686f17f0 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -28,6 +28,10 @@ static struct efi_device_path *bootefi_device_path;
 /**
  * efi_init_obj_list() - Initialize and populate EFI object list
  *
+ * TODO(sjg at chromium.org): Move this to a dynamic list based on driver model,
+ * so that it does not need to be created before running EFI applications
+ * and updates when devices change.
+ *
  * @return 0 if OK, -ve on error (in which case it prints a message)
  */
 static efi_status_t efi_init_obj_list(void)
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 05/16] efi: Correct header order in efi_memory

[Simon Glass]

The headers are not in the correct order. Fix this. Also drop libfdt_env.h
since it is not needed.

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

Changes in v2:
- Update commit message to dropping libfdt_env.h

 lib/efi_loader/efi_memory.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index d47759e08e..e95896ca0a 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -8,12 +8,11 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <inttypes.h>
 #include <malloc.h>
+#include <watchdog.h>
 #include <asm/global_data.h>
-#include <libfdt_env.h>
 #include <linux/list_sort.h>
-#include <inttypes.h>
-#include <watchdog.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 06/16] efi: sandbox: Adjust memory usage for sandbox

[Simon Glass]

With sandbox the U-Boot code is not mapped into the sandbox memory range
so does not need to be excluded when allocating EFI memory. Update the EFI
memory init code to take account of that.

Also use mapmem instead of a cast to convert a memory address to a
pointer.

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

Changes in v2:
- Update to use mapmem instead of a cast

 lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index e95896ca0a..3ad58d8930 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -10,6 +10,7 @@
 #include <efi_loader.h>
 #include <inttypes.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <watchdog.h>
 #include <asm/global_data.h>
 #include <linux/list_sort.h>
@@ -366,7 +367,7 @@ efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
 	r = efi_allocate_pages(0, pool_type, num_pages, &t);
 
 	if (r == EFI_SUCCESS) {
-		struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
+		struct efi_pool_allocation *alloc = map_sysmem(t, size);
 		alloc->num_pages = num_pages;
 		*buffer = alloc->data;
 	}
@@ -460,18 +461,22 @@ int efi_memory_init(void)
 
 	efi_add_known_memory();
 
-	/* Add U-Boot */
-	uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK;
-	uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
-	efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
-
-	/* Add Runtime Services */
-	runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
-	runtime_end = (ulong)&__efi_runtime_stop;
-	runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
-	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
-	efi_add_memory_map(runtime_start, runtime_pages,
-			   EFI_RUNTIME_SERVICES_CODE, false);
+	if (!IS_ENABLED(CONFIG_SANDBOX)) {
+		/* Add U-Boot */
+		uboot_start = (gd->start_addr_sp - uboot_stack_size) &
+				~EFI_PAGE_MASK;
+		uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
+		efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA,
+				   false);
+
+		/* Add Runtime Services */
+		runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
+		runtime_end = (ulong)&__efi_runtime_stop;
+		runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
+		runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
+		efi_add_memory_map(runtime_start, runtime_pages,
+				   EFI_RUNTIME_SERVICES_CODE, false);
+	}
 
 #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
 	/* Request a 32bit 64MB bounce buffer region */
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 07/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

At present this code casts addresses to pointers so cannot be used with
sandbox. Update it to use mapmem instead.

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

Changes in v2: None

 lib/smbios.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/lib/smbios.c b/lib/smbios.c
index 8f19ad89c1..56481d448d 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -7,6 +7,7 @@
  */
 
 #include <common.h>
+#include <mapmem.h>
 #include <smbios.h>
 #include <tables_csum.h>
 #include <version.h>
@@ -75,9 +76,10 @@ static int smbios_string_table_len(char *start)
 
 static int smbios_write_type0(ulong *current, int handle)
 {
-	struct smbios_type0 *t = (struct smbios_type0 *)*current;
+	struct smbios_type0 *t;
 	int len = sizeof(struct smbios_type0);
 
+	t = map_sysmem(*current, len);
 	memset(t, 0, sizeof(struct smbios_type0));
 	fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
 	t->vendor = smbios_add_string(t->eos, "U-Boot");
@@ -104,16 +106,18 @@ static int smbios_write_type0(ulong *current, int handle)
 
 	len = t->length + smbios_string_table_len(t->eos);
 	*current += len;
+	unmap_sysmem(t);
 
 	return len;
 }
 
 static int smbios_write_type1(ulong *current, int handle)
 {
-	struct smbios_type1 *t = (struct smbios_type1 *)*current;
+	struct smbios_type1 *t;
 	int len = sizeof(struct smbios_type1);
 	char *serial_str = env_get("serial#");
 
+	t = map_sysmem(*current, len);
 	memset(t, 0, sizeof(struct smbios_type1));
 	fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle);
 	t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
@@ -125,15 +129,17 @@ static int smbios_write_type1(ulong *current, int handle)
 
 	len = t->length + smbios_string_table_len(t->eos);
 	*current += len;
+	unmap_sysmem(t);
 
 	return len;
 }
 
 static int smbios_write_type2(ulong *current, int handle)
 {
-	struct smbios_type2 *t = (struct smbios_type2 *)*current;
+	struct smbios_type2 *t;
 	int len = sizeof(struct smbios_type2);
 
+	t = map_sysmem(*current, len);
 	memset(t, 0, sizeof(struct smbios_type2));
 	fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle);
 	t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
@@ -143,15 +149,17 @@ static int smbios_write_type2(ulong *current, int handle)
 
 	len = t->length + smbios_string_table_len(t->eos);
 	*current += len;
+	unmap_sysmem(t);
 
 	return len;
 }
 
 static int smbios_write_type3(ulong *current, int handle)
 {
-	struct smbios_type3 *t = (struct smbios_type3 *)*current;
+	struct smbios_type3 *t;
 	int len = sizeof(struct smbios_type3);
 
+	t = map_sysmem(*current, len);
 	memset(t, 0, sizeof(struct smbios_type3));
 	fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle);
 	t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
@@ -163,6 +171,7 @@ static int smbios_write_type3(ulong *current, int handle)
 
 	len = t->length + smbios_string_table_len(t->eos);
 	*current += len;
+	unmap_sysmem(t);
 
 	return len;
 }
@@ -201,9 +210,10 @@ static void smbios_write_type4_dm(struct smbios_type4 *t)
 
 static int smbios_write_type4(ulong *current, int handle)
 {
-	struct smbios_type4 *t = (struct smbios_type4 *)*current;
+	struct smbios_type4 *t;
 	int len = sizeof(struct smbios_type4);
 
+	t = map_sysmem(*current, len);
 	memset(t, 0, sizeof(struct smbios_type4));
 	fill_smbios_header(t, SMBIOS_PROCESSOR_INFORMATION, len, handle);
 	t->processor_type = SMBIOS_PROCESSOR_TYPE_CENTRAL;
@@ -217,32 +227,37 @@ static int smbios_write_type4(ulong *current, int handle)
 
 	len = t->length + smbios_string_table_len(t->eos);
 	*current += len;
+	unmap_sysmem(t);
 
 	return len;
 }
 
 static int smbios_write_type32(ulong *current, int handle)
 {
-	struct smbios_type32 *t = (struct smbios_type32 *)*current;
+	struct smbios_type32 *t;
 	int len = sizeof(struct smbios_type32);
 
+	t = map_sysmem(*current, len);
 	memset(t, 0, sizeof(struct smbios_type32));
 	fill_smbios_header(t, SMBIOS_SYSTEM_BOOT_INFORMATION, len, handle);
 
 	*current += len;
+	unmap_sysmem(t);
 
 	return len;
 }
 
 static int smbios_write_type127(ulong *current, int handle)
 {
-	struct smbios_type127 *t = (struct smbios_type127 *)*current;
+	struct smbios_type127 *t;
 	int len = sizeof(struct smbios_type127);
 
+	t = map_sysmem(*current, len);
 	memset(t, 0, sizeof(struct smbios_type127));
 	fill_smbios_header(t, SMBIOS_END_OF_TABLE, len, handle);
 
 	*current += len;
+	unmap_sysmem(t);
 
 	return len;
 }
@@ -271,7 +286,7 @@ ulong write_smbios_table(ulong addr)
 	/* 16 byte align the table address */
 	addr = ALIGN(addr, 16);
 
-	se = (struct smbios_entry *)(uintptr_t)addr;
+	se = map_sysmem(addr, sizeof(struct smbios_entry));
 	memset(se, 0, sizeof(struct smbios_entry));
 
 	addr += sizeof(struct smbios_entry);
@@ -280,9 +295,13 @@ ulong write_smbios_table(ulong addr)
 
 	/* populate minimum required tables */
 	for (i = 0; i < ARRAY_SIZE(smbios_write_funcs); i++) {
-		int tmp = smbios_write_funcs[i]((ulong *)&addr, handle++);
+		ulong *ptr = map_sysmem(addr, 0);
+		int tmp;
+
+		tmp = smbios_write_funcs[i](ptr, handle++);
 		max_struct_size = max(max_struct_size, tmp);
 		len += tmp;
+		unmap_sysmem(ptr);
 	}
 
 	memcpy(se->anchor, "_SM_", 4);
@@ -300,6 +319,7 @@ ulong write_smbios_table(ulong addr)
 	isize = sizeof(struct smbios_entry) - SMBIOS_INTERMEDIATE_OFFSET;
 	se->intermediate_checksum = table_compute_checksum(istart, isize);
 	se->checksum = table_compute_checksum(se, sizeof(struct smbios_entry));
+	unmap_sysmem(se);
 
 	return addr;
 }
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 08/16] sandbox: Add a setjmp() implementation

[Simon Glass]

Add an implementation of setjmp() and longjmp() which rely on the
underlying host C library. Since we cannot know how large the jump buffer
needs to be, pick something that should be suitable and check it at
runtime. At present we need access to the underlying struct as well.

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

Changes in v2: None

 arch/sandbox/cpu/cpu.c            | 13 +++++++++++++
 arch/sandbox/cpu/os.c             | 17 +++++++++++++++++
 arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++++++++++
 include/os.h                      | 21 +++++++++++++++++++++
 4 files changed, 72 insertions(+)
 create mode 100644 arch/sandbox/include/asm/setjmp.h

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index 66c3a6a88a..8b397c7168 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -9,6 +9,7 @@
 #include <libfdt.h>
 #include <os.h>
 #include <asm/io.h>
+#include <asm/setjmp.h>
 #include <asm/state.h>
 #include <dm/root.h>
 
@@ -164,3 +165,15 @@ ulong timer_get_boot_us(void)
 
 	return (count - base_count) / 1000;
 }
+
+int setjmp(jmp_buf jmp)
+{
+	return os_setjmp((ulong *)jmp, sizeof(jmp));
+}
+
+void longjmp(jmp_buf jmp, int ret)
+{
+	os_longjmp((ulong *)jmp, ret);
+	while (1)
+		;
+}
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index c524957b6c..e9200192c6 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -7,6 +7,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <getopt.h>
+#include <setjmp.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -617,3 +618,19 @@ void os_localtime(struct rtc_time *rt)
 	rt->tm_yday = tm->tm_yday;
 	rt->tm_isdst = tm->tm_isdst;
 }
+
+int os_setjmp(ulong *jmp, int size)
+{
+	if (size < sizeof(jmp_buf)) {
+		printf("setjmp: jmpbuf is too small (%d bytes, need %d)\n",
+		       size, sizeof(jmp_buf));
+		return -ENOSPC;
+	}
+
+	return setjmp((struct __jmp_buf_tag *)jmp);
+}
+
+void os_longjmp(ulong *jmp, int ret)
+{
+	longjmp((struct __jmp_buf_tag *)jmp, ret);
+}
diff --git a/arch/sandbox/include/asm/setjmp.h b/arch/sandbox/include/asm/setjmp.h
new file mode 100644
index 0000000000..e25f50107c
--- /dev/null
+++ b/arch/sandbox/include/asm/setjmp.h
@@ -0,0 +1,21 @@
+/*
+ * (C) Copyright 2016
+ * Alexander Graf <agraf@suse.de>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef _SETJMP_H_
+#define _SETJMP_H_	1
+
+struct jmp_buf_data {
+	/* We're not sure how long this should be */
+	ulong data[32];
+};
+
+typedef struct jmp_buf_data jmp_buf[1];
+
+int setjmp(jmp_buf jmp);
+__noreturn void longjmp(jmp_buf jmp, int ret);
+
+#endif /* _SETJMP_H_ */
diff --git a/include/os.h b/include/os.h
index 2bf4bdb1b8..ad1836ac9f 100644
--- a/include/os.h
+++ b/include/os.h
@@ -310,4 +310,25 @@ int os_spl_to_uboot(const char *fname);
  */
 void os_localtime(struct rtc_time *rt);
 
+/**
+ * os_setjmp() - Call setjmp()
+ *
+ * Call the host system's setjmp() function.
+ *
+ * @jmp: Buffer to store current execution state
+ * @size: Size of buffer
+ * @return normal setjmp() value if OK, -ENOSPC if @size is too small
+ */
+int os_setjmp(ulong *jmp, int size);
+
+/**
+ * os_longjmp() - Call longjmp()
+ *
+ * Call the host system's longjmp() function.
+ *
+ * @jmp: Buffer where previous execution state was stored
+ * @ret: Value to pass to longjmp()
+ */
+void os_longjmp(ulong *jmp, int ret);
+
 #endif
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 09/16] efi: sandbox: Add required linker sections

[Simon Glass]

The EFI loader code requires certain linker sections to exist. Add these
for sandbox so that the EFI loader code will link.

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

Changes in v2: None

 arch/sandbox/cpu/u-boot.lds | 29 +++++++++++++++++++++++++++++
 arch/sandbox/lib/Makefile   |  2 +-
 arch/sandbox/lib/sections.c | 12 ++++++++++++
 3 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 arch/sandbox/lib/sections.c

diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds
index 7e92b4ac66..38a74bcaf4 100644
--- a/arch/sandbox/cpu/u-boot.lds
+++ b/arch/sandbox/cpu/u-boot.lds
@@ -19,6 +19,35 @@ SECTIONS
 	__u_boot_sandbox_option_end = .;
 
 	__bss_start = .;
+
+		.__efi_runtime_start : {
+		*(.__efi_runtime_start)
+	}
+
+	.efi_runtime : {
+		*(efi_runtime_text)
+		*(efi_runtime_data)
+	}
+
+	.__efi_runtime_stop : {
+		*(.__efi_runtime_stop)
+	}
+
+	.efi_runtime_rel_start :
+	{
+		*(.__efi_runtime_rel_start)
+	}
+
+	.efi_runtime_rel : {
+		*(.relefi_runtime_text)
+		*(.relefi_runtime_data)
+	}
+
+	.efi_runtime_rel_stop :
+	{
+		*(.__efi_runtime_rel_stop)
+	}
+
 }
 
 INSERT BEFORE .data;
diff --git a/arch/sandbox/lib/Makefile b/arch/sandbox/lib/Makefile
index 2e7802feac..d2171579b7 100644
--- a/arch/sandbox/lib/Makefile
+++ b/arch/sandbox/lib/Makefile
@@ -7,7 +7,7 @@
 # SPDX-License-Identifier:	GPL-2.0+
 #
 
-obj-y	+= interrupts.o
+obj-y	+= interrupts.o sections.o
 ifndef CONFIG_SPL_BUILD
 obj-$(CONFIG_PCI)	+= pci_io.o
 endif
diff --git a/arch/sandbox/lib/sections.c b/arch/sandbox/lib/sections.c
new file mode 100644
index 0000000000..6455e0f088
--- /dev/null
+++ b/arch/sandbox/lib/sections.c
@@ -0,0 +1,12 @@
+/*
+ * Copyright 2013 Albert ARIBAUD <albert.u.boot@aribaud.net>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+char __efi_runtime_start[0] __attribute__((section(".__efi_runtime_start")));
+char __efi_runtime_stop[0] __attribute__((section(".__efi_runtime_stop")));
+char __efi_runtime_rel_start[0]
+		__attribute__((section(".__efi_runtime_rel_start")));
+char __efi_runtime_rel_stop[0]
+		__attribute__((section(".__efi_runtime_rel_stop")));
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 10/16] efi: sandbox: Add distroboot support

[Simon Glass]

With sandbox these values depend on the host system. Let's assume that it
is x86_64 for now.

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

Changes in v2: None

 include/config_distro_bootcmd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 5c469a23fa..6772d2d404 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -242,7 +242,7 @@
 #elif defined(CONFIG_ARM)
 #define BOOTENV_EFI_PXE_ARCH "0xa"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
-#elif defined(CONFIG_X86)
+#elif defined(CONFIG_X86) || defined(CONFIG_SANDBOX)
 /* Always assume we're running 64bit */
 #define BOOTENV_EFI_PXE_ARCH "0x7"
 #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 11/16] Define board_quiesce_devices() in a shared location

[Simon Glass]

This undocumented function relies on arch-specific code to declare a nop
weak version. Add the weak function in common code instead to avoid having
to duplicate the same function in each arch.

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

Changes in v2: None

 arch/arm/include/asm/u-boot-arm.h | 1 -
 arch/x86/include/asm/u-boot-x86.h | 1 -
 arch/x86/lib/bootm.c              | 4 ----
 common/bootm.c                    | 4 ++++
 include/bootm.h                   | 2 ++
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/u-boot-arm.h b/arch/arm/include/asm/u-boot-arm.h
index ef4fca68ee..73ccf41f8c 100644
--- a/arch/arm/include/asm/u-boot-arm.h
+++ b/arch/arm/include/asm/u-boot-arm.h
@@ -38,7 +38,6 @@ int	arch_early_init_r(void);
 
 /* board/.../... */
 int	board_init(void);
-void	board_quiesce_devices(void);
 
 /* cpu/.../interrupt.c */
 int	arch_interrupt_init	(void);
diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
index 187fe5fd8c..d2e1426042 100644
--- a/arch/x86/include/asm/u-boot-x86.h
+++ b/arch/x86/include/asm/u-boot-x86.h
@@ -85,7 +85,6 @@ static inline __attribute__((no_instrument_function)) uint64_t rdtsc(void)
 /* board/... */
 void timer_set_tsc_base(uint64_t new_base);
 uint64_t timer_get_tsc(void);
-void board_quiesce_devices(void);
 
 void quick_ram_check(void);
 
diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
index e548cdbed5..60ae043f24 100644
--- a/arch/x86/lib/bootm.c
+++ b/arch/x86/lib/bootm.c
@@ -33,10 +33,6 @@ int arch_fixup_fdt(void *blob)
 	return 0;
 }
 
-__weak void board_quiesce_devices(void)
-{
-}
-
 void bootm_announce_and_cleanup(void)
 {
 	printf("\nStarting kernel ...\n\n");
diff --git a/common/bootm.c b/common/bootm.c
index 9493a306cd..9fba0472ac 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -47,6 +47,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 				   char * const argv[], bootm_headers_t *images,
 				   ulong *os_data, ulong *os_len);
 
+__weak void board_quiesce_devices(void)
+{
+}
+
 #ifdef CONFIG_LMB
 static void boot_start_lmb(bootm_headers_t *images)
 {
diff --git a/include/bootm.h b/include/bootm.h
index 49813772ce..76b6ab42e6 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -73,4 +73,6 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
 		       void *load_buf, void *image_buf, ulong image_len,
 		       uint unc_len, ulong *load_end);
 
+void board_quiesce_devices(void);
+
 #endif
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 12/16] Add a comment for board_quiesce_devices()

[Simon Glass]

This exported function should have a comment describing what it does. Also
it should really be removed in favour of device_remove(), which handles
this sort of thing now. Add a comment with a TODO.

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

Changes in v2: None

 include/bootm.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/bootm.h b/include/bootm.h
index 76b6ab42e6..caf331f54b 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -73,6 +73,12 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
 		       void *load_buf, void *image_buf, ulong image_len,
 		       uint unc_len, ulong *load_end);
 
+/*
+ * boards should define this to disable devices when EFI exits from boot
+ * services.
+ *
+ * TODO(sjg at chromium.org>): Update this to use driver model's device_remove().
+ */
 void board_quiesce_devices(void);
 
 #endif
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 13/16] efi: sandbox: Add relocation constants

[Simon Glass]

Add these so that we can build the EFI loader for sandbox. The values are
for x86_64 so potentially bogus. But we don't support relocation within
sandbox anyway.

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

Changes in v2: None

 lib/efi_loader/efi_runtime.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 8104e08c46..468dc12565 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -48,6 +48,9 @@ static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void);
 #include <asm/elf.h>
 #define R_RELATIVE	R_386_RELATIVE
 #define R_MASK		0xffULL
+#elif defined(CONFIG_SANDBOX)
+#define R_RELATIVE	8
+#define R_MASK		0xffffffffULL
 #else
 #error Need to add relocation awareness
 #endif
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 14/16] efi: Add a comment about duplicated ELF constants

[Simon Glass]

These constants are defined in arch-specific code but redefined here. Add
a TODO to clean this up.

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

Changes in v2: None

 lib/efi_loader/efi_runtime.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index 468dc12565..8313e9c4e0 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -37,6 +37,10 @@ static efi_status_t __efi_runtime EFIAPI efi_invalid_parameter(void);
 #define EFI_CACHELINE_SIZE 128
 #endif
 
+/*
+ * TODO(sjg at chromium.org): These defines and structs should come from the elf.
+ * header for each arch (or a generic header) rather than being repeated here.
+ */
 #if defined(CONFIG_ARM64)
 #define R_RELATIVE	1027
 #define R_MASK		0xffffffffULL
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 15/16] efi: sandbox: Enable EFI loader builder for sandbox

[Simon Glass]

This allows this feature to build within sandbox. This is for testing
purposes only since it is not possible for sandbox to load native code.

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

Changes in v2: None

 lib/efi_loader/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index d2b6327119..dee0a96a98 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -1,6 +1,6 @@
 config EFI_LOADER
 	bool "Support running EFI Applications in U-Boot"
-	depends on (ARM || X86) && OF_LIBFDT
+	depends on (ARM || X86 || SANDBOX) && OF_LIBFDT
 	default y
 	help
 	  Select this option if you want to run EFI applications (like grub2)
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 16/16] efi: sandbox: Add a simple 'bootefi test' command

[Simon Glass]

This jumps to test code which can call directly into the EFI support. It
does not need a separate image so it is easy to write tests with it.

For now the test just outputs a message. To try it:

./sandbox/u-boot -c "bootefi test"
U-Boot 2017.09-00204-g696c9855fe (Sep 17 2017 - 16:43:53 -0600)

DRAM:  128 MiB
MMC:
Using default environment

In:    serial
Out:   serial
Err:   serial
SCSI:  Net:   No ethernet found.
IDE:   Bus 0: not available
Found 0 disks
Hello, world!
Test passed

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

Changes in v2:
- Rebase to master

 cmd/bootefi.c             | 25 +++++++++++++++++++++++--
 configs/sandbox_defconfig |  1 +
 include/efi_loader.h      |  3 +++
 lib/efi_loader/Kconfig    | 10 ++++++++++
 lib/efi_loader/Makefile   |  1 +
 lib/efi_loader/efi_test.c | 17 +++++++++++++++++
 6 files changed, 55 insertions(+), 2 deletions(-)
 create mode 100644 lib/efi_loader/efi_test.c

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index a4686f17f0..a77d8b256e 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -148,13 +148,11 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
 	struct efi_object loaded_image_info_obj = {};
 	struct efi_device_path *memdp = NULL;
 	ulong ret;
-
 	ulong (*entry)(void *image_handle, struct efi_system_table *st)
 		asmlinkage;
 	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
 	const efi_guid_t fdt_guid = EFI_FDT_GUID;
 	bootm_headers_t img = { 0 };
-	int ret;
 
 	/*
 	 * Special case for efi payload not loaded from disk, such as
@@ -318,6 +316,29 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		memcpy((char *)addr, __efi_helloworld_begin, size);
 	} else
 #endif
+	if (IS_ENABLED(CONFIG_BOOTEFI_TEST) && !strcmp(argv[1], "test")) {
+		int ret;
+
+		/* Initialize and populate EFI object list */
+		if (efi_init_obj_list())
+			return CMD_RET_FAILURE;
+
+		struct efi_loaded_image loaded_image_info = {};
+		struct efi_object loaded_image_info_obj = {};
+
+		efi_setup_loaded_image(&loaded_image_info,
+				       &loaded_image_info_obj,
+				       bootefi_device_path, bootefi_image_path);
+
+		ret = efi_test(&loaded_image_info, &systab);
+		if (ret) {
+			printf("Test failed: err=%d\n", ret);
+			return CMD_RET_FAILURE;
+		}
+		printf("Test passed\n");
+
+		return 0;
+	}
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 	if (!strcmp(argv[1], "selftest")) {
 		struct efi_loaded_image loaded_image_info = {};
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 08eec8ecc6..ccf142c51d 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -194,3 +194,4 @@ CONFIG_UT_TIME=y
 CONFIG_UT_DM=y
 CONFIG_UT_ENV=y
 CONFIG_UT_OVERLAY=y
+CONFIG_CMD_BOOTEFI_SELFTEST=y
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 35f8f84401..90018fbe9e 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -335,6 +335,9 @@ efi_status_t EFIAPI efi_set_variable(s16 *variable_name,
 void *efi_bootmgr_load(struct efi_device_path **device_path,
 		       struct efi_device_path **file_path);
 
+/* Perform EFI tests */
+int efi_test(efi_handle_t image_handle, struct efi_system_table *systab);
+
 #else /* defined(EFI_LOADER) && !defined(CONFIG_SPL_BUILD) */
 
 /* Without CONFIG_EFI_LOADER we don't have a runtime section, stub it out */
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index dee0a96a98..659b2b18f4 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -16,3 +16,13 @@ config EFI_LOADER_BOUNCE_BUFFER
 	  Some hardware does not support DMA to full 64bit addresses. For this
 	  hardware we can create a bounce buffer so that payloads don't have to
 	  worry about platform details.
+
+config BOOTEFI_TEST
+	bool "Provide a test for the EFI loader"
+	depends on EFI_LOADER && SANDBOX
+	default y
+	help
+	  Provides a test of the EFI loader functionality accessed via the
+	  command line ('bootefi test'). This runs within U-Boot so does not
+	  need a separate EFI application to work. It aims to include coverage
+	  of all EFI code which can be accessed within sandbox.
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index ddb978f650..14ef85cc67 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_DM_VIDEO) += efi_gop.o
 obj-$(CONFIG_PARTITIONS) += efi_disk.o
 obj-$(CONFIG_NET) += efi_net.o
 obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
+obj-$(CONFIG_BOOTEFI_TEST) += efi_test.o
diff --git a/lib/efi_loader/efi_test.c b/lib/efi_loader/efi_test.c
new file mode 100644
index 0000000000..3fdce78b05
--- /dev/null
+++ b/lib/efi_loader/efi_test.c
@@ -0,0 +1,17 @@
+/*
+ * Copyright (c) 2017, Google Inc. All rights reserved.
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <efi_api.h>
+
+int efi_test(efi_handle_t image_handle, struct efi_system_table *systable)
+{
+	struct efi_simple_text_output_protocol *con_out = systable->con_out;
+
+	con_out->output_string(con_out, L"Hello, world!\n");
+
+	return 0;
+}
-- 
2.15.0.531.g2ccb3012c9-goog

[U-Boot] [PATCH v2 16/16] efi: sandbox: Add a simple 'bootefi test' command

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> This jumps to test code which can call directly into the EFI support. It
> does not need a separate image so it is easy to write tests with it.
> 
> For now the test just outputs a message. To try it:

Hello Simon,

why do we need "bootefi test"?
What do you plan to do here which we cannot do in "bootefi selftest?
I think we should unify both.

Best regards

Heinrich

> 
> ./sandbox/u-boot -c "bootefi test"
> U-Boot 2017.09-00204-g696c9855fe (Sep 17 2017 - 16:43:53 -0600)
> 
> DRAM:  128 MiB
> MMC:
> Using default environment
> 
> In:    serial
> Out:   serial
> Err:   serial
> SCSI:  Net:   No ethernet found.
> IDE:   Bus 0: not available
> Found 0 disks
> Hello, world!
> Test passed
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Rebase to master
> 
>   cmd/bootefi.c             | 25 +++++++++++++++++++++++--
>   configs/sandbox_defconfig |  1 +
>   include/efi_loader.h      |  3 +++
>   lib/efi_loader/Kconfig    | 10 ++++++++++
>   lib/efi_loader/Makefile   |  1 +
>   lib/efi_loader/efi_test.c | 17 +++++++++++++++++
>   6 files changed, 55 insertions(+), 2 deletions(-)
>   create mode 100644 lib/efi_loader/efi_test.c
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index a4686f17f0..a77d8b256e 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -148,13 +148,11 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>   	struct efi_object loaded_image_info_obj = {};
>   	struct efi_device_path *memdp = NULL;
>   	ulong ret;
> -
>   	ulong (*entry)(void *image_handle, struct efi_system_table *st)
>   		asmlinkage;
>   	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
>   	const efi_guid_t fdt_guid = EFI_FDT_GUID;
>   	bootm_headers_t img = { 0 };
> -	int ret;
>   
>   	/*
>   	 * Special case for efi payload not loaded from disk, such as
> @@ -318,6 +316,29 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   		memcpy((char *)addr, __efi_helloworld_begin, size);
>   	} else
>   #endif
> +	if (IS_ENABLED(CONFIG_BOOTEFI_TEST) && !strcmp(argv[1], "test")) {
> +		int ret;
> +
> +		/* Initialize and populate EFI object list */
> +		if (efi_init_obj_list())
> +			return CMD_RET_FAILURE;
> +
> +		struct efi_loaded_image loaded_image_info = {};
> +		struct efi_object loaded_image_info_obj = {};
> +
> +		efi_setup_loaded_image(&loaded_image_info,
> +				       &loaded_image_info_obj,
> +				       bootefi_device_path, bootefi_image_path);
> +
> +		ret = efi_test(&loaded_image_info, &systab);
> +		if (ret) {
> +			printf("Test failed: err=%d\n", ret);
> +			return CMD_RET_FAILURE;
> +		}
> +		printf("Test passed\n");
> +
> +		return 0;
> +	}
>   #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>   	if (!strcmp(argv[1], "selftest")) {
>   		struct efi_loaded_image loaded_image_info = {};
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index 08eec8ecc6..ccf142c51d 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -194,3 +194,4 @@ CONFIG_UT_TIME=y
>   CONFIG_UT_DM=y
>   CONFIG_UT_ENV=y
>   CONFIG_UT_OVERLAY=y
> +CONFIG_CMD_BOOTEFI_SELFTEST=y
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 35f8f84401..90018fbe9e 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -335,6 +335,9 @@ efi_status_t EFIAPI efi_set_variable(s16 *variable_name,
>   void *efi_bootmgr_load(struct efi_device_path **device_path,
>   		       struct efi_device_path **file_path);
>   
> +/* Perform EFI tests */
> +int efi_test(efi_handle_t image_handle, struct efi_system_table *systab);
> +
>   #else /* defined(EFI_LOADER) && !defined(CONFIG_SPL_BUILD) */
>   
>   /* Without CONFIG_EFI_LOADER we don't have a runtime section, stub it out */
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index dee0a96a98..659b2b18f4 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -16,3 +16,13 @@ config EFI_LOADER_BOUNCE_BUFFER
>   	  Some hardware does not support DMA to full 64bit addresses. For this
>   	  hardware we can create a bounce buffer so that payloads don't have to
>   	  worry about platform details.
> +
> +config BOOTEFI_TEST
> +	bool "Provide a test for the EFI loader"
> +	depends on EFI_LOADER && SANDBOX
> +	default y
> +	help
> +	  Provides a test of the EFI loader functionality accessed via the
> +	  command line ('bootefi test'). This runs within U-Boot so does not
> +	  need a separate EFI application to work. It aims to include coverage
> +	  of all EFI code which can be accessed within sandbox.
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index ddb978f650..14ef85cc67 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -23,3 +23,4 @@ obj-$(CONFIG_DM_VIDEO) += efi_gop.o
>   obj-$(CONFIG_PARTITIONS) += efi_disk.o
>   obj-$(CONFIG_NET) += efi_net.o
>   obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> +obj-$(CONFIG_BOOTEFI_TEST) += efi_test.o
> diff --git a/lib/efi_loader/efi_test.c b/lib/efi_loader/efi_test.c
> new file mode 100644
> index 0000000000..3fdce78b05
> --- /dev/null
> +++ b/lib/efi_loader/efi_test.c
> @@ -0,0 +1,17 @@
> +/*
> + * Copyright (c) 2017, Google Inc. All rights reserved.
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <efi_api.h>
> +
> +int efi_test(efi_handle_t image_handle, struct efi_system_table *systable)
> +{
> +	struct efi_simple_text_output_protocol *con_out = systable->con_out;
> +
> +	con_out->output_string(con_out, L"Hello, world!\n");
> +
> +	return 0;
> +}
> 

[U-Boot] [PATCH v2 16/16] efi: sandbox: Add a simple 'bootefi test' command

[Simon Glass]

Hi Heinrich,

On 4 December 2017 at 14:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 12/04/2017 10:28 PM, Simon Glass wrote:
>>
>> This jumps to test code which can call directly into the EFI support. It
>> does not need a separate image so it is easy to write tests with it.
>>
>> For now the test just outputs a message. To try it:
>
>
> Hello Simon,
>
> why do we need "bootefi test"?
> What do you plan to do here which we cannot do in "bootefi selftest?
> I think we should unify both.

It runs the hello world test directly without going through a separate app.

This is just a starting point, to get EFI running on sandbox. Of
course after that we can do more work to make it run the proper tests.

Regards,
Simon

[...]

[U-Boot] [PATCH v2 16/16] efi: sandbox: Add a simple 'bootefi test' command

[Heinrich Schuchardt]

On 12/04/2017 11:07 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On 4 December 2017 at 14:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>> On 12/04/2017 10:28 PM, Simon Glass wrote:
>>>
>>> This jumps to test code which can call directly into the EFI support. It
>>> does not need a separate image so it is easy to write tests with it.
>>>
>>> For now the test just outputs a message. To try it:
>>
>>
>> Hello Simon,
>>
>> why do we need "bootefi test"?
>> What do you plan to do here which we cannot do in "bootefi selftest?
>> I think we should unify both.
> 
> It runs the hello world test directly without going through a separate app.
> 
> This is just a starting point, to get EFI running on sandbox. Of
> course after that we can do more work to make it run the proper tests.

bootefi selftest does not run a separate app either but stays in the 
same executable.

Regards

Heinrich

> 
> Regards,
> Simon
> 
> [...]
> 

[U-Boot] [PATCH v2 01/16] efi: Update efi_smbios_register() to return error code

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> This function can fail but gives no indication of failure. Update it to
> return an error when something goes wrong.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Update return type of efi_smbios_register() to efi_status_t
> - Use return value of efi_install_configuration_table
> 
>   include/efi_loader.h        | 9 ++++++++-
>   lib/efi_loader/efi_smbios.c | 7 ++++---
>   2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 1b92edbd77..35f8f84401 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -164,7 +164,14 @@ int efi_gop_register(void);
>   /* Called by bootefi to make the network interface available */
>   int efi_net_register(void);
>   /* Called by bootefi to make SMBIOS tables available */
> -void efi_smbios_register(void);
> +/**
> + * efi_smbios_register() - write out SMBIOS tables
> + *
> + * Called by bootefi to make SMBIOS tables available
> + *
> + * @return 0 if OK, -ENOMEM if no memory is available for the tables
> + */
> +efi_status_t efi_smbios_register(void);
>   
>   struct efi_simple_file_system_protocol *
>   efi_fs_from_path(struct efi_device_path *fp);
> diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
> index ac412e7362..67f71892ca 100644
> --- a/lib/efi_loader/efi_smbios.c
> +++ b/lib/efi_loader/efi_smbios.c
> @@ -13,7 +13,7 @@
>   
>   static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
>   
> -void efi_smbios_register(void)
> +efi_status_t efi_smbios_register(void)
>   {
>   	/* Map within the low 32 bits, to allow for 32bit SMBIOS tables */
>   	uint64_t dmi = 0xffffffff;
> @@ -22,11 +22,12 @@ void efi_smbios_register(void)
>   	int memtype = EFI_RUNTIME_SERVICES_DATA;
>   
>   	if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS)

Please, return the value returned from efi_allocate_pages(). The 
function has a lot of different failure modes.

> -		return;
> +		return EFI_OUT_OF_RESOURCES;
>   
>   	/* Generate SMBIOS tables */
>   	write_smbios_table(dmi);
We should add a comment explaining why we do not use the return value of 
write_smbios_table(). My understanding is:
write_smbios_table returns dmi rounded up to a multiple of 16.
efi_allocate_pages returns a 4096 aligned address. So we do expect that 
the return value equals the parameter apssed to write_smbios_table().

Best regards

Heinrich

>   
>   	/* And expose them to our EFI payload */
> -	efi_install_configuration_table(&smbios_guid, (void*)(uintptr_t)dmi);
> +	return efi_install_configuration_table(&smbios_guid,
> +					       (void *)(uintptr_t)dmi);
>   }
> 

[U-Boot] [PATCH v2 03/16] efi: Add error checking for efi_init_obj_list()

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> This function calls a function which can fail. Print a message in this
> case and abort the boot, rather than silently continuing to boot, which
> will certainly fail.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Change return type of efi_init_obj_list() to efi_status_t
> 
>   cmd/bootefi.c | 32 +++++++++++++++++++++++++-------
>   1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 17b26e6f4e..a2138f6075 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized;
>   static struct efi_device_path *bootefi_image_path;
>   static struct efi_device_path *bootefi_device_path;
>   
> -/* Initialize and populate EFI object list */
> -static void efi_init_obj_list(void)
> +/**
> + * efi_init_obj_list() - Initialize and populate EFI object list
> + *
> + * @return 0 if OK, -ve on error (in which case it prints a message)
> + */
> +static efi_status_t efi_init_obj_list(void)
>   {
> +	efi_status_t ret;
> +
>   	if (efi_obj_list_initalized)
> -		return;
> +		return 0;
>   	efi_obj_list_initalized = 1;
>   
>   	efi_console_register();
> @@ -43,12 +49,19 @@ static void efi_init_obj_list(void)
>   	efi_net_register();

This function can also fail. Same is true for efi_gop_register.

>   #endif
>   #ifdef CONFIG_GENERATE_SMBIOS_TABLE
> -	efi_smbios_register();
> +	ret = efi_smbios_register();
> +	if (ret)

if (ret != EFI_SUCCESS)

> +		goto error;
>   #endif
>   
>   	/* Initialize EFI runtime services */
>   	efi_reset_system_init();
>   	efi_get_time_init();
> +
> +	return EFI_SUCCESS;
> +error:
> +	printf("Error: Cannot set up EFI object list (err=%d)\n", ret);

Now we have some objects initialized and others not.
Before returning we should clean up.
We should free all allocated objects.

Best regards

Heinrich

> +	return ret;
>   }
>   
>   static void *copy_fdt(void *fdt)
> @@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>   	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
>   	const efi_guid_t fdt_guid = EFI_FDT_GUID;
>   	bootm_headers_t img = { 0 };
> +	int ret;
>   
>   	/*
>   	 * Special case for efi payload not loaded from disk, such as
> @@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>   		"{ro,boot}(blob)0000000000000000");
>   
>   	/* Initialize and populate EFI object list */
> -	efi_init_obj_list();
> +	ret = efi_init_obj_list();
> +	if (ret)
> +		return ret;
>   
>   	/* Call our payload! */
>   	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
> @@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   		 */
>   		efi_save_gd();
>   		/* Initialize and populate EFI object list */
> -		if (!efi_obj_list_initalized)
> -			efi_init_obj_list();
> +		if (!efi_obj_list_initalized && efi_init_obj_list())
> +			return CMD_RET_FAILURE;
> +
>   		loaded_image_info.device_handle = bootefi_device_path;
>   		loaded_image_info.file_path = bootefi_image_path;
> +
>   		return efi_selftest(&loaded_image_info, &systab);
>   	} else
>   #endif
> 

[U-Boot] [PATCH v2 02/16] efi: Move the init check inside efi_init_obj_list()

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> Rather than having the caller check this variable and the callee set it,
> move all access to the variable inside the function. This reduces the
> logic needed to call efi_init_obj_list().
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> 
> Changes in v2: None
> 
>   cmd/bootefi.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 478bc116e2..17b26e6f4e 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -28,6 +28,8 @@ static struct efi_device_path *bootefi_device_path;
>   /* Initialize and populate EFI object list */
>   static void efi_init_obj_list(void)
>   {
> +	if (efi_obj_list_initalized)
> +		return;
>   	efi_obj_list_initalized = 1;
>   
>   	efi_console_register();
> @@ -208,6 +210,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>   	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>   		"{ro,boot}(blob)0000000000000000");
>   
> +	/* Initialize and populate EFI object list */
> +	efi_init_obj_list();

If you add it here where do you remove the old call? We do not want to 
do the assignments twice.

> +
>   	/* Call our payload! */
>   	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
>   
> @@ -310,6 +315,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   		/* Initialize and populate EFI object list */
>   		if (!efi_obj_list_initalized)
>   			efi_init_obj_list();
> +		loaded_image_info.device_handle = bootefi_device_path;
> +		loaded_image_info.file_path = bootefi_image_path;

Same.

Best regards

Heinrich

>   		return efi_selftest(&loaded_image_info, &systab);
>   	} else
>   #endif
> 

[U-Boot] [PATCH v2 05/16] efi: Correct header order in efi_memory

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> The headers are not in the correct order. Fix this. Also drop libfdt_env.h
> since it is not needed.

What do you consider as correct order? Do you mean sorted alphabetically?
Includes should be self sufficient and the correct execution be 
independent of the sequence.

Best regards

Heinrich

> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Update commit message to dropping libfdt_env.h
> 
>   lib/efi_loader/efi_memory.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index d47759e08e..e95896ca0a 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -8,12 +8,11 @@
>   
>   #include <common.h>
>   #include <efi_loader.h>
> +#include <inttypes.h>
>   #include <malloc.h>
> +#include <watchdog.h>
>   #include <asm/global_data.h>
> -#include <libfdt_env.h>
>   #include <linux/list_sort.h>
> -#include <inttypes.h>
> -#include <watchdog.h>
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> 

[U-Boot] [PATCH v2 10/16] efi: sandbox: Add distroboot support

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> With sandbox these values depend on the host system. Let's assume that it
> is x86_64 for now.

How would you run this on a Pinebook (arm64)?

Shouldn't we strive to make sandbox work on any architecture?

Best regards

Heinrich


> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2: None
> 
>   include/config_distro_bootcmd.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 5c469a23fa..6772d2d404 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -242,7 +242,7 @@
>   #elif defined(CONFIG_ARM)
>   #define BOOTENV_EFI_PXE_ARCH "0xa"
>   #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00010:UNDI:003000"
> -#elif defined(CONFIG_X86)
> +#elif defined(CONFIG_X86) || defined(CONFIG_SANDBOX)
>   /* Always assume we're running 64bit */
>   #define BOOTENV_EFI_PXE_ARCH "0x7"
>   #define BOOTENV_EFI_PXE_VCI "PXEClient:Arch:00007:UNDI:003000"
> 

[U-Boot] [PATCH v2 11/16] Define board_quiesce_devices() in a shared location

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> This undocumented function relies on arch-specific code to declare a nop

Undocumented code is unsatisfactory. Please, add a comment to the function.

Best regards

Heinrich

> weak version. Add the weak function in common code instead to avoid having
> to duplicate the same function in each arch.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2: None
> 
>   arch/arm/include/asm/u-boot-arm.h | 1 -
>   arch/x86/include/asm/u-boot-x86.h | 1 -
>   arch/x86/lib/bootm.c              | 4 ----
>   common/bootm.c                    | 4 ++++
>   include/bootm.h                   | 2 ++
>   5 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/include/asm/u-boot-arm.h b/arch/arm/include/asm/u-boot-arm.h
> index ef4fca68ee..73ccf41f8c 100644
> --- a/arch/arm/include/asm/u-boot-arm.h
> +++ b/arch/arm/include/asm/u-boot-arm.h
> @@ -38,7 +38,6 @@ int	arch_early_init_r(void);
>   
>   /* board/.../... */
>   int	board_init(void);
> -void	board_quiesce_devices(void);
>   
>   /* cpu/.../interrupt.c */
>   int	arch_interrupt_init	(void);
> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
> index 187fe5fd8c..d2e1426042 100644
> --- a/arch/x86/include/asm/u-boot-x86.h
> +++ b/arch/x86/include/asm/u-boot-x86.h
> @@ -85,7 +85,6 @@ static inline __attribute__((no_instrument_function)) uint64_t rdtsc(void)
>   /* board/... */
>   void timer_set_tsc_base(uint64_t new_base);
>   uint64_t timer_get_tsc(void);
> -void board_quiesce_devices(void);
>   
>   void quick_ram_check(void);
>   
> diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
> index e548cdbed5..60ae043f24 100644
> --- a/arch/x86/lib/bootm.c
> +++ b/arch/x86/lib/bootm.c
> @@ -33,10 +33,6 @@ int arch_fixup_fdt(void *blob)
>   	return 0;
>   }
>   
> -__weak void board_quiesce_devices(void)
> -{
> -}
> -
>   void bootm_announce_and_cleanup(void)
>   {
>   	printf("\nStarting kernel ...\n\n");
> diff --git a/common/bootm.c b/common/bootm.c
> index 9493a306cd..9fba0472ac 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -47,6 +47,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>   				   char * const argv[], bootm_headers_t *images,
>   				   ulong *os_data, ulong *os_len);
>   
> +__weak void board_quiesce_devices(void)
> +{
> +}
> +
>   #ifdef CONFIG_LMB
>   static void boot_start_lmb(bootm_headers_t *images)
>   {
> diff --git a/include/bootm.h b/include/bootm.h
> index 49813772ce..76b6ab42e6 100644
> --- a/include/bootm.h
> +++ b/include/bootm.h
> @@ -73,4 +73,6 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type,
>   		       void *load_buf, void *image_buf, ulong image_len,
>   		       uint unc_len, ulong *load_end);
>   
> +void board_quiesce_devices(void);
> +
>   #endif
> 

[U-Boot] [PATCH v2 11/16] Define board_quiesce_devices() in a shared location

[Heinrich Schuchardt]

On 12/04/2017 11:41 PM, Heinrich Schuchardt wrote:
> 
> 
> On 12/04/2017 10:28 PM, Simon Glass wrote:
>> This undocumented function relies on arch-specific code to declare a nop
> 
> Undocumented code is unsatisfactory. Please, add a comment to the function.

Sorry I missed you put that in a separate patch (12/16).

BR

> 
> Best regards
> 
> Heinrich
> 
>> weak version. Add the weak function in common code instead to avoid 
>> having
>> to duplicate the same function in each arch.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v2: None
>>
>>   arch/arm/include/asm/u-boot-arm.h | 1 -
>>   arch/x86/include/asm/u-boot-x86.h | 1 -
>>   arch/x86/lib/bootm.c              | 4 ----
>>   common/bootm.c                    | 4 ++++
>>   include/bootm.h                   | 2 ++
>>   5 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/u-boot-arm.h 
>> b/arch/arm/include/asm/u-boot-arm.h
>> index ef4fca68ee..73ccf41f8c 100644
>> --- a/arch/arm/include/asm/u-boot-arm.h
>> +++ b/arch/arm/include/asm/u-boot-arm.h
>> @@ -38,7 +38,6 @@ int    arch_early_init_r(void);
>>   /* board/.../... */
>>   int    board_init(void);
>> -void    board_quiesce_devices(void);
>>   /* cpu/.../interrupt.c */
>>   int    arch_interrupt_init    (void);
>> diff --git a/arch/x86/include/asm/u-boot-x86.h 
>> b/arch/x86/include/asm/u-boot-x86.h
>> index 187fe5fd8c..d2e1426042 100644
>> --- a/arch/x86/include/asm/u-boot-x86.h
>> +++ b/arch/x86/include/asm/u-boot-x86.h
>> @@ -85,7 +85,6 @@ static inline 
>> __attribute__((no_instrument_function)) uint64_t rdtsc(void)
>>   /* board/... */
>>   void timer_set_tsc_base(uint64_t new_base);
>>   uint64_t timer_get_tsc(void);
>> -void board_quiesce_devices(void);
>>   void quick_ram_check(void);
>> diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
>> index e548cdbed5..60ae043f24 100644
>> --- a/arch/x86/lib/bootm.c
>> +++ b/arch/x86/lib/bootm.c
>> @@ -33,10 +33,6 @@ int arch_fixup_fdt(void *blob)
>>       return 0;
>>   }
>> -__weak void board_quiesce_devices(void)
>> -{
>> -}
>> -
>>   void bootm_announce_and_cleanup(void)
>>   {
>>       printf("\nStarting kernel ...\n\n");
>> diff --git a/common/bootm.c b/common/bootm.c
>> index 9493a306cd..9fba0472ac 100644
>> --- a/common/bootm.c
>> +++ b/common/bootm.c
>> @@ -47,6 +47,10 @@ static const void *boot_get_kernel(cmd_tbl_t 
>> *cmdtp, int flag, int argc,
>>                      char * const argv[], bootm_headers_t *images,
>>                      ulong *os_data, ulong *os_len);
>> +__weak void board_quiesce_devices(void)
>> +{
>> +}
>> +
>>   #ifdef CONFIG_LMB
>>   static void boot_start_lmb(bootm_headers_t *images)
>>   {
>> diff --git a/include/bootm.h b/include/bootm.h
>> index 49813772ce..76b6ab42e6 100644
>> --- a/include/bootm.h
>> +++ b/include/bootm.h
>> @@ -73,4 +73,6 @@ int bootm_decomp_image(int comp, ulong load, ulong 
>> image_start, int type,
>>                  void *load_buf, void *image_buf, ulong image_len,
>>                  uint unc_len, ulong *load_end);
>> +void board_quiesce_devices(void);
>> +
>>   #endif
>>
> 

[U-Boot] [PATCH v2 10/16] efi: sandbox: Add distroboot support

[Simon Glass]

Hi Heinrich,

On 4 December 2017 at 15:37, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 12/04/2017 10:28 PM, Simon Glass wrote:
>>
>> With sandbox these values depend on the host system. Let's assume that it
>> is x86_64 for now.
>
>
> How would you run this on a Pinebook (arm64)?
>
> Shouldn't we strive to make sandbox work on any architecture?
>

Yes that would be good.

But I think getting it going on x86 is a good first step and will
cover the vast majority of use cases (e.g. for travis-ci testing).

Regards,
Simon

[U-Boot] [PATCH v2 05/16] efi: Correct header order in efi_memory

[Simon Glass]

Hi Heinrich,

On 4 December 2017 at 15:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 12/04/2017 10:28 PM, Simon Glass wrote:
>>
>> The headers are not in the correct order. Fix this. Also drop libfdt_env.h
>> since it is not needed.
>
>
> What do you consider as correct order? Do you mean sorted alphabetically?
> Includes should be self sufficient and the correct execution be independent
> of the sequence.

https://www.denx.de/wiki/U-Boot/CodingStyle

If we need libfdt we should include libfdt.h I think.

Regards,
Simon

[U-Boot] [PATCH v2 03/16] efi: Add error checking for efi_init_obj_list()

[Simon Glass]

Hi Heinrich,

On 4 December 2017 at 15:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 12/04/2017 10:28 PM, Simon Glass wrote:
>>
>> This function calls a function which can fail. Print a message in this
>> case and abort the boot, rather than silently continuing to boot, which
>> will certainly fail.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v2:
>> - Change return type of efi_init_obj_list() to efi_status_t
>>
>>   cmd/bootefi.c | 32 +++++++++++++++++++++++++-------
>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 17b26e6f4e..a2138f6075 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized;
>>   static struct efi_device_path *bootefi_image_path;
>>   static struct efi_device_path *bootefi_device_path;
>>   -/* Initialize and populate EFI object list */
>> -static void efi_init_obj_list(void)
>> +/**
>> + * efi_init_obj_list() - Initialize and populate EFI object list
>> + *
>> + * @return 0 if OK, -ve on error (in which case it prints a message)
>> + */
>> +static efi_status_t efi_init_obj_list(void)
>>   {
>> +       efi_status_t ret;
>> +
>>         if (efi_obj_list_initalized)
>> -               return;
>> +               return 0;
>>         efi_obj_list_initalized = 1;
>>         efi_console_register();
>> @@ -43,12 +49,19 @@ static void efi_init_obj_list(void)
>>         efi_net_register();
>
>
> This function can also fail. Same is true for efi_gop_register.
>
>>   #endif
>>   #ifdef CONFIG_GENERATE_SMBIOS_TABLE
>> -       efi_smbios_register();
>> +       ret = efi_smbios_register();
>> +       if (ret)
>
>
> if (ret != EFI_SUCCESS)
>

Please can we avoid this obfuscation? It really pains me to see what I
consider to be stupidity in the code. Can we just agree that success
is 0 in U-Boot/?

>> +               goto error;
>>   #endif
>>         /* Initialize EFI runtime services */
>>         efi_reset_system_init();
>>         efi_get_time_init();
>> +
>> +       return EFI_SUCCESS;
>> +error:
>> +       printf("Error: Cannot set up EFI object list (err=%d)\n", ret);
>
>
> Now we have some objects initialized and others not.
> Before returning we should clean up.
> We should free all allocated objects.

OK let me take another look at this.

>
> Best regards
>
> Heinrich
>
>
>> +       return ret;
>>   }
>>     static void *copy_fdt(void *fdt)
>> @@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void
>> *fdt,
>>         ulong fdt_pages, fdt_size, fdt_start, fdt_end;
>>         const efi_guid_t fdt_guid = EFI_FDT_GUID;
>>         bootm_headers_t img = { 0 };
>> +       int ret;
>>         /*
>>          * Special case for efi payload not loaded from disk, such as
>> @@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void
>> *fdt,
>>                 "{ro,boot}(blob)0000000000000000");
>>         /* Initialize and populate EFI object list */
>> -       efi_init_obj_list();
>> +       ret = efi_init_obj_list();
>> +       if (ret)
>> +               return ret;
>>         /* Call our payload! */
>>         debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__,
>> (long)entry);
>> @@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag,
>> int argc, char * const argv[])
>>                  */
>>                 efi_save_gd();
>>                 /* Initialize and populate EFI object list */
>> -               if (!efi_obj_list_initalized)
>> -                       efi_init_obj_list();
>> +               if (!efi_obj_list_initalized && efi_init_obj_list())
>> +                       return CMD_RET_FAILURE;
>> +
>>                 loaded_image_info.device_handle = bootefi_device_path;
>>                 loaded_image_info.file_path = bootefi_image_path;
>> +
>>                 return efi_selftest(&loaded_image_info, &systab);
>>         } else
>>   #endif
>>
>


Regards,
Simon

[U-Boot] [PATCH v2 03/16] efi: Add error checking for efi_init_obj_list()

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> This function calls a function which can fail. Print a message in this
> case and abort the boot, rather than silently continuing to boot, which
> will certainly fail.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Change return type of efi_init_obj_list() to efi_status_t
> 
>  cmd/bootefi.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 17b26e6f4e..a2138f6075 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized;
>  static struct efi_device_path *bootefi_image_path;
>  static struct efi_device_path *bootefi_device_path;
>  
> -/* Initialize and populate EFI object list */
> -static void efi_init_obj_list(void)
> +/**
> + * efi_init_obj_list() - Initialize and populate EFI object list
> + *
> + * @return 0 if OK, -ve on error (in which case it prints a message)
> + */
> +static efi_status_t efi_init_obj_list(void)
>  {
> +	efi_status_t ret;
> +
>  	if (efi_obj_list_initalized)
> -		return;
> +		return 0;
>  	efi_obj_list_initalized = 1;
>  
>  	efi_console_register();
> @@ -43,12 +49,19 @@ static void efi_init_obj_list(void)
>  	efi_net_register();
>  #endif
>  #ifdef CONFIG_GENERATE_SMBIOS_TABLE
> -	efi_smbios_register();
> +	ret = efi_smbios_register();
> +	if (ret)
> +		goto error;
>  #endif
>  
>  	/* Initialize EFI runtime services */
>  	efi_reset_system_init();
>  	efi_get_time_init();
> +
> +	return EFI_SUCCESS;
> +error:
> +	printf("Error: Cannot set up EFI object list (err=%d)\n", ret);

warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has
type ‘efi_status_t {aka long unsigned int}’ [-Wformat=]

Please, use %lu.

Regards

Heinrich

> +	return ret;
>  }
>  
>  static void *copy_fdt(void *fdt)
> @@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>  	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
>  	const efi_guid_t fdt_guid = EFI_FDT_GUID;
>  	bootm_headers_t img = { 0 };
> +	int ret;
>  
>  	/*
>  	 * Special case for efi payload not loaded from disk, such as
> @@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>  		"{ro,boot}(blob)0000000000000000");
>  
>  	/* Initialize and populate EFI object list */
> -	efi_init_obj_list();
> +	ret = efi_init_obj_list();
> +	if (ret)
> +		return ret;
>  
>  	/* Call our payload! */
>  	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
> @@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		 */
>  		efi_save_gd();
>  		/* Initialize and populate EFI object list */
> -		if (!efi_obj_list_initalized)
> -			efi_init_obj_list();
> +		if (!efi_obj_list_initalized && efi_init_obj_list())
> +			return CMD_RET_FAILURE;
> +
>  		loaded_image_info.device_handle = bootefi_device_path;
>  		loaded_image_info.file_path = bootefi_image_path;
> +
>  		return efi_selftest(&loaded_image_info, &systab);
>  	} else
>  #endif
> 

[U-Boot] [PATCH v2 08/16] sandbox: Add a setjmp() implementation

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> Add an implementation of setjmp() and longjmp() which rely on the
> underlying host C library. Since we cannot know how large the jump buffer
> needs to be, pick something that should be suitable and check it at
> runtime. At present we need access to the underlying struct as well.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2: None
> 
>  arch/sandbox/cpu/cpu.c            | 13 +++++++++++++
>  arch/sandbox/cpu/os.c             | 17 +++++++++++++++++
>  arch/sandbox/include/asm/setjmp.h | 21 +++++++++++++++++++++
>  include/os.h                      | 21 +++++++++++++++++++++
>  4 files changed, 72 insertions(+)
>  create mode 100644 arch/sandbox/include/asm/setjmp.h
> 
> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> index 66c3a6a88a..8b397c7168 100644
> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
> @@ -9,6 +9,7 @@
>  #include <libfdt.h>
>  #include <os.h>
>  #include <asm/io.h>
> +#include <asm/setjmp.h>
>  #include <asm/state.h>
>  #include <dm/root.h>
>  
> @@ -164,3 +165,15 @@ ulong timer_get_boot_us(void)
>  
>  	return (count - base_count) / 1000;
>  }
> +
> +int setjmp(jmp_buf jmp)
> +{
> +	return os_setjmp((ulong *)jmp, sizeof(jmp));

This produces a warning:
‘sizeof’ on array function parameter ‘jmp’ will return size of ‘struct
jmp_buf_data *’ [-Wsizeof-array-argument]

Did you mean sizeof(struct jmp_buf_data)?

> +}
> +
> +void longjmp(jmp_buf jmp, int ret)
> +{
> +	os_longjmp((ulong *)jmp, ret);
> +	while (1)
> +		;
> +}
> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> index c524957b6c..e9200192c6 100644
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> @@ -7,6 +7,7 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <getopt.h>
> +#include <setjmp.h>
>  #include <stdio.h>
>  #include <stdint.h>
>  #include <stdlib.h>
> @@ -617,3 +618,19 @@ void os_localtime(struct rtc_time *rt)
>  	rt->tm_yday = tm->tm_yday;
>  	rt->tm_isdst = tm->tm_isdst;
>  }
> +
> +int os_setjmp(ulong *jmp, int size)
> +{
> +	if (size < sizeof(jmp_buf)) {
> +		printf("setjmp: jmpbuf is too small (%d bytes, need %d)\n",
> +		       size, sizeof(struct jmp_buf_data));

Shouldn't this be sizeof(*jmp_buf)?

> +		return -ENOSPC;
> +	}
> +
> +	return setjmp((struct __jmp_buf_tag *)jmp);
> +}
> +
> +void os_longjmp(ulong *jmp, int ret)
> +{
> +	longjmp((struct __jmp_buf_tag *)jmp, ret);
> +}
> diff --git a/arch/sandbox/include/asm/setjmp.h b/arch/sandbox/include/asm/setjmp.h
> new file mode 100644
> index 0000000000..e25f50107c
> --- /dev/null
> +++ b/arch/sandbox/include/asm/setjmp.h
> @@ -0,0 +1,21 @@
> +/*
> + * (C) Copyright 2016
> + * Alexander Graf <agraf@suse.de>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef _SETJMP_H_
> +#define _SETJMP_H_	1
> +
> +struct jmp_buf_data {
> +	/* We're not sure how long this should be */
> +	ulong data[32];

Isn't this buffer is too short?

sizeof(jmp_buf)
===============
amd64: 200 bytes
arm64: 392 bytes
armhf: 392 bytes

Please, check the alignment. On Windows amd64 a jump buffer must be 16
byte aligned.

Best regards

Heinrich

> +};
> +
> +typedef struct jmp_buf_data jmp_buf[1];
> +
> +int setjmp(jmp_buf jmp);
> +__noreturn void longjmp(jmp_buf jmp, int ret);
> +
> +#endif /* _SETJMP_H_ */
> diff --git a/include/os.h b/include/os.h
> index 2bf4bdb1b8..ad1836ac9f 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -310,4 +310,25 @@ int os_spl_to_uboot(const char *fname);
>   */
>  void os_localtime(struct rtc_time *rt);
>  
> +/**
> + * os_setjmp() - Call setjmp()
> + *
> + * Call the host system's setjmp() function.
> + *
> + * @jmp: Buffer to store current execution state
> + * @size: Size of buffer
> + * @return normal setjmp() value if OK, -ENOSPC if @size is too small
> + */
> +int os_setjmp(ulong *jmp, int size);
> +
> +/**
> + * os_longjmp() - Call longjmp()
> + *
> + * Call the host system's longjmp() function.
> + *
> + * @jmp: Buffer where previous execution state was stored
> + * @ret: Value to pass to longjmp()
> + */
> +void os_longjmp(ulong *jmp, int ret);
> +
>  #endif
> 

[U-Boot] [PATCH v2 06/16] efi: sandbox: Adjust memory usage for sandbox

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> With sandbox the U-Boot code is not mapped into the sandbox memory range
> so does not need to be excluded when allocating EFI memory. Update the EFI
> memory init code to take account of that.
> 
> Also use mapmem instead of a cast to convert a memory address to a
> pointer.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Update to use mapmem instead of a cast
> 
>  lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index e95896ca0a..3ad58d8930 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -10,6 +10,7 @@
>  #include <efi_loader.h>
>  #include <inttypes.h>
>  #include <malloc.h>
> +#include <mapmem.h>
>  #include <watchdog.h>
>  #include <asm/global_data.h>
>  #include <linux/list_sort.h>
> @@ -366,7 +367,7 @@ efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
>  	r = efi_allocate_pages(0, pool_type, num_pages, &t);
>  
>  	if (r == EFI_SUCCESS) {
> -		struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
> +		struct efi_pool_allocation *alloc = map_sysmem(t, size);
>  		alloc->num_pages = num_pages;
>  		*buffer = alloc->data;
>  	}
> @@ -460,18 +461,22 @@ int efi_memory_init(void)
>  
>  	efi_add_known_memory();
>  
> -	/* Add U-Boot */
> -	uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK;
> -	uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
> -	efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
> -
> -	/* Add Runtime Services */
> -	runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
> -	runtime_end = (ulong)&__efi_runtime_stop;
> -	runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
> -	runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
> -	efi_add_memory_map(runtime_start, runtime_pages,
> -			   EFI_RUNTIME_SERVICES_CODE, false);
> +	if (!IS_ENABLED(CONFIG_SANDBOX)) {
> +		/* Add U-Boot */
> +		uboot_start = (gd->start_addr_sp - uboot_stack_size) &
> +				~EFI_PAGE_MASK;
> +		uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
> +		efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA,
> +				   false);
> +
> +		/* Add Runtime Services */
> +		runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK;
> +		runtime_end = (ulong)&__efi_runtime_stop;
> +		runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK;
> +		runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;
> +		efi_add_memory_map(runtime_start, runtime_pages,
> +				   EFI_RUNTIME_SERVICES_CODE, false);
> +	}
>  
>  #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
>  	/* Request a 32bit 64MB bounce buffer region */
> 

bootefi selftest shows an error. You should setup the EFI memory for
CONFIG_SANDBOX=y too.

Setting up 'ExitBootServices'
Setting up 'ExitBootServices' succeeded
lib/efi_selftest/efi_selftest.c(53):
ERROR: GetMemoryMap did not return EFI_SUCCESS

Best regards

Heinrich

[U-Boot] [PATCH v2 07/16] sandbox: smbios: Update to support sandbox

[Heinrich Schuchardt]

On 12/04/2017 10:28 PM, Simon Glass wrote:
> At present this code casts addresses to pointers so cannot be used with
> sandbox. Update it to use mapmem instead.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2: None
> 
>  lib/smbios.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/smbios.c b/lib/smbios.c
> index 8f19ad89c1..56481d448d 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <common.h>
> +#include <mapmem.h>
>  #include <smbios.h>
>  #include <tables_csum.h>
>  #include <version.h>
> @@ -75,9 +76,10 @@ static int smbios_string_table_len(char *start)
>  
>  static int smbios_write_type0(ulong *current, int handle)
>  {
> -	struct smbios_type0 *t = (struct smbios_type0 *)*current;
> +	struct smbios_type0 *t;
>  	int len = sizeof(struct smbios_type0);
>  
> +	t = map_sysmem(*current, len);
>  	memset(t, 0, sizeof(struct smbios_type0));
>  	fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
>  	t->vendor = smbios_add_string(t->eos, "U-Boot");
> @@ -104,16 +106,18 @@ static int smbios_write_type0(ulong *current, int handle)
>  
>  	len = t->length + smbios_string_table_len(t->eos);
>  	*current += len;
> +	unmap_sysmem(t);
>  
>  	return len;
>  }
>  
>  static int smbios_write_type1(ulong *current, int handle)
>  {
> -	struct smbios_type1 *t = (struct smbios_type1 *)*current;
> +	struct smbios_type1 *t;
>  	int len = sizeof(struct smbios_type1);
>  	char *serial_str = env_get("serial#");
>  
> +	t = map_sysmem(*current, len);
>  	memset(t, 0, sizeof(struct smbios_type1));
>  	fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle);
>  	t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
> @@ -125,15 +129,17 @@ static int smbios_write_type1(ulong *current, int handle)
>  
>  	len = t->length + smbios_string_table_len(t->eos);
>  	*current += len;
> +	unmap_sysmem(t);
>  
>  	return len;
>  }
>  
>  static int smbios_write_type2(ulong *current, int handle)
>  {
> -	struct smbios_type2 *t = (struct smbios_type2 *)*current;
> +	struct smbios_type2 *t;
>  	int len = sizeof(struct smbios_type2);
>  
> +	t = map_sysmem(*current, len);
>  	memset(t, 0, sizeof(struct smbios_type2));
>  	fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle);
>  	t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
> @@ -143,15 +149,17 @@ static int smbios_write_type2(ulong *current, int handle)
>  
>  	len = t->length + smbios_string_table_len(t->eos);
>  	*current += len;
> +	unmap_sysmem(t);
>  
>  	return len;
>  }
>  
>  static int smbios_write_type3(ulong *current, int handle)
>  {
> -	struct smbios_type3 *t = (struct smbios_type3 *)*current;
> +	struct smbios_type3 *t;
>  	int len = sizeof(struct smbios_type3);
>  
> +	t = map_sysmem(*current, len);
>  	memset(t, 0, sizeof(struct smbios_type3));
>  	fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle);
>  	t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
> @@ -163,6 +171,7 @@ static int smbios_write_type3(ulong *current, int handle)
>  
>  	len = t->length + smbios_string_table_len(t->eos);
>  	*current += len;
> +	unmap_sysmem(t);
>  
>  	return len;
>  }
> @@ -201,9 +210,10 @@ static void smbios_write_type4_dm(struct smbios_type4 *t)
>  
>  static int smbios_write_type4(ulong *current, int handle)
>  {
> -	struct smbios_type4 *t = (struct smbios_type4 *)*current;
> +	struct smbios_type4 *t;
>  	int len = sizeof(struct smbios_type4);
>  
> +	t = map_sysmem(*current, len);
>  	memset(t, 0, sizeof(struct smbios_type4));
>  	fill_smbios_header(t, SMBIOS_PROCESSOR_INFORMATION, len, handle);
>  	t->processor_type = SMBIOS_PROCESSOR_TYPE_CENTRAL;
> @@ -217,32 +227,37 @@ static int smbios_write_type4(ulong *current, int handle)
>  
>  	len = t->length + smbios_string_table_len(t->eos);
>  	*current += len;
> +	unmap_sysmem(t);
>  
>  	return len;
>  }
>  
>  static int smbios_write_type32(ulong *current, int handle)
>  {
> -	struct smbios_type32 *t = (struct smbios_type32 *)*current;
> +	struct smbios_type32 *t;
>  	int len = sizeof(struct smbios_type32);
>  
> +	t = map_sysmem(*current, len);
>  	memset(t, 0, sizeof(struct smbios_type32));
>  	fill_smbios_header(t, SMBIOS_SYSTEM_BOOT_INFORMATION, len, handle);
>  
>  	*current += len;
> +	unmap_sysmem(t);
>  
>  	return len;
>  }
>  
>  static int smbios_write_type127(ulong *current, int handle)
>  {
> -	struct smbios_type127 *t = (struct smbios_type127 *)*current;
> +	struct smbios_type127 *t;
>  	int len = sizeof(struct smbios_type127);
>  
> +	t = map_sysmem(*current, len);
>  	memset(t, 0, sizeof(struct smbios_type127));
>  	fill_smbios_header(t, SMBIOS_END_OF_TABLE, len, handle);
>  
>  	*current += len;
> +	unmap_sysmem(t);
>  
>  	return len;
>  }
> @@ -271,7 +286,7 @@ ulong write_smbios_table(ulong addr)
>  	/* 16 byte align the table address */
>  	addr = ALIGN(addr, 16);
>  
> -	se = (struct smbios_entry *)(uintptr_t)addr;
> +	se = map_sysmem(addr, sizeof(struct smbios_entry));
>  	memset(se, 0, sizeof(struct smbios_entry));
>  
>  	addr += sizeof(struct smbios_entry);
> @@ -280,9 +295,13 @@ ulong write_smbios_table(ulong addr)
>  
>  	/* populate minimum required tables */
>  	for (i = 0; i < ARRAY_SIZE(smbios_write_funcs); i++) {
> -		int tmp = smbios_write_funcs[i]((ulong *)&addr, handle++);
> +		ulong *ptr = map_sysmem(addr, 0);

This replaces &addr by addr and breaks booting on ARM64.

Did you mean:
		ulong *ptr = map_sysmem((phys_addr_t)&addr, 0);

The coding would get much easier to maintain if the map_sysmem()
function would be eliminated from U-Boot.

Why don't you simply use the address range that mmap() has provided?
This would avoid double book keeping.

Regards

Heinrich

> +		int tmp;
> +
> +		tmp = smbios_write_funcs[i](ptr, handle++);
>  		max_struct_size = max(max_struct_size, tmp);
>  		len += tmp;
> +		unmap_sysmem(ptr);
>  	}
>  
>  	memcpy(se->anchor, "_SM_", 4);
> @@ -300,6 +319,7 @@ ulong write_smbios_table(ulong addr)
>  	isize = sizeof(struct smbios_entry) - SMBIOS_INTERMEDIATE_OFFSET;
>  	se->intermediate_checksum = table_compute_checksum(istart, isize);
>  	se->checksum = table_compute_checksum(se, sizeof(struct smbios_entry));
> +	unmap_sysmem(se);
>  
>  	return addr;
>  }
> 

[U-Boot] [PATCH v2 16/16] efi: sandbox: Add a simple 'bootefi test' command

[Simon Glass]

Hi Heinrich,

On 4 December 2017 at 15:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 12/04/2017 11:07 PM, Simon Glass wrote:
>>
>> Hi Heinrich,
>>
>> On 4 December 2017 at 14:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>>
>>>
>>> On 12/04/2017 10:28 PM, Simon Glass wrote:
>>>>
>>>>
>>>> This jumps to test code which can call directly into the EFI support. It
>>>> does not need a separate image so it is easy to write tests with it.
>>>>
>>>> For now the test just outputs a message. To try it:
>>>
>>>
>>>
>>> Hello Simon,
>>>
>>> why do we need "bootefi test"?
>>> What do you plan to do here which we cannot do in "bootefi selftest?
>>> I think we should unify both.
>>
>>
>> It runs the hello world test directly without going through a separate app.
>>
>> This is just a starting point, to get EFI running on sandbox. Of
>> course after that we can do more work to make it run the proper tests.
>
>
> bootefi selftest does not run a separate app either but stays in the same executable.

I still see some value in having a simple test. The existing one seems
to require a U-Boot reboot...?

Regards,
Simon

[U-Boot] [PATCH v2 01/16] efi: Update efi_smbios_register() to return error code

[Simon Glass]

Hi Heinrich,

On 4 December 2017 at 15:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 12/04/2017 10:28 PM, Simon Glass wrote:
>>
>> This function can fail but gives no indication of failure. Update it to
>> return an error when something goes wrong.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v2:
>> - Update return type of efi_smbios_register() to efi_status_t
>> - Use return value of efi_install_configuration_table
>>
>>   include/efi_loader.h        | 9 ++++++++-
>>   lib/efi_loader/efi_smbios.c | 7 ++++---
>>   2 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 1b92edbd77..35f8f84401 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -164,7 +164,14 @@ int efi_gop_register(void);
>>   /* Called by bootefi to make the network interface available */
>>   int efi_net_register(void);
>>   /* Called by bootefi to make SMBIOS tables available */
>> -void efi_smbios_register(void);
>> +/**
>> + * efi_smbios_register() - write out SMBIOS tables
>> + *
>> + * Called by bootefi to make SMBIOS tables available
>> + *
>> + * @return 0 if OK, -ENOMEM if no memory is available for the tables
>> + */
>> +efi_status_t efi_smbios_register(void);
>>     struct efi_simple_file_system_protocol *
>>   efi_fs_from_path(struct efi_device_path *fp);
>> diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
>> index ac412e7362..67f71892ca 100644
>> --- a/lib/efi_loader/efi_smbios.c
>> +++ b/lib/efi_loader/efi_smbios.c
>> @@ -13,7 +13,7 @@
>>     static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
>>   -void efi_smbios_register(void)
>> +efi_status_t efi_smbios_register(void)
>>   {
>>         /* Map within the low 32 bits, to allow for 32bit SMBIOS tables */
>>         uint64_t dmi = 0xffffffff;
>> @@ -22,11 +22,12 @@ void efi_smbios_register(void)
>>         int memtype = EFI_RUNTIME_SERVICES_DATA;
>>         if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS)
>
>
> Please, return the value returned from efi_allocate_pages(). The function
> has a lot of different failure modes.

OK, will do. But looking at efi_allocate_pages() there is no
documentation as to the return value.

>
>> -               return;
>> +               return EFI_OUT_OF_RESOURCES;
>>         /* Generate SMBIOS tables */
>>         write_smbios_table(dmi);
>
> We should add a comment explaining why we do not use the return value of
> write_smbios_table(). My understanding is:
> write_smbios_table returns dmi rounded up to a multiple of 16.
> efi_allocate_pages returns a 4096 aligned address. So we do expect that the
> return value equals the parameter apssed to write_smbios_table().

OK will do.

Regards,
Simon

[U-Boot] [PATCH v2 07/16] sandbox: smbios: Update to support sandbox

[Simon Glass]

Hi Heinrich,

On 18 February 2018 at 05:14, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 12/04/2017 10:28 PM, Simon Glass wrote:
>> At present this code casts addresses to pointers so cannot be used with
>> sandbox. Update it to use mapmem instead.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v2: None
>>
>>  lib/smbios.c | 38 +++++++++++++++++++++++++++++---------
>>  1 file changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/smbios.c b/lib/smbios.c
>> index 8f19ad89c1..56481d448d 100644
>> --- a/lib/smbios.c
>> +++ b/lib/smbios.c
>> @@ -7,6 +7,7 @@
>>   */
>>
>>  #include <common.h>
>> +#include <mapmem.h>
>>  #include <smbios.h>
>>  #include <tables_csum.h>
>>  #include <version.h>
>> @@ -75,9 +76,10 @@ static int smbios_string_table_len(char *start)
>>
>>  static int smbios_write_type0(ulong *current, int handle)
>>  {
>> -     struct smbios_type0 *t = (struct smbios_type0 *)*current;
>> +     struct smbios_type0 *t;
>>       int len = sizeof(struct smbios_type0);
>>
>> +     t = map_sysmem(*current, len);
>>       memset(t, 0, sizeof(struct smbios_type0));
>>       fill_smbios_header(t, SMBIOS_BIOS_INFORMATION, len, handle);
>>       t->vendor = smbios_add_string(t->eos, "U-Boot");
>> @@ -104,16 +106,18 @@ static int smbios_write_type0(ulong *current, int handle)
>>
>>       len = t->length + smbios_string_table_len(t->eos);
>>       *current += len;
>> +     unmap_sysmem(t);
>>
>>       return len;
>>  }
>>
>>  static int smbios_write_type1(ulong *current, int handle)
>>  {
>> -     struct smbios_type1 *t = (struct smbios_type1 *)*current;
>> +     struct smbios_type1 *t;
>>       int len = sizeof(struct smbios_type1);
>>       char *serial_str = env_get("serial#");
>>
>> +     t = map_sysmem(*current, len);
>>       memset(t, 0, sizeof(struct smbios_type1));
>>       fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle);
>>       t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
>> @@ -125,15 +129,17 @@ static int smbios_write_type1(ulong *current, int handle)
>>
>>       len = t->length + smbios_string_table_len(t->eos);
>>       *current += len;
>> +     unmap_sysmem(t);
>>
>>       return len;
>>  }
>>
>>  static int smbios_write_type2(ulong *current, int handle)
>>  {
>> -     struct smbios_type2 *t = (struct smbios_type2 *)*current;
>> +     struct smbios_type2 *t;
>>       int len = sizeof(struct smbios_type2);
>>
>> +     t = map_sysmem(*current, len);
>>       memset(t, 0, sizeof(struct smbios_type2));
>>       fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle);
>>       t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
>> @@ -143,15 +149,17 @@ static int smbios_write_type2(ulong *current, int handle)
>>
>>       len = t->length + smbios_string_table_len(t->eos);
>>       *current += len;
>> +     unmap_sysmem(t);
>>
>>       return len;
>>  }
>>
>>  static int smbios_write_type3(ulong *current, int handle)
>>  {
>> -     struct smbios_type3 *t = (struct smbios_type3 *)*current;
>> +     struct smbios_type3 *t;
>>       int len = sizeof(struct smbios_type3);
>>
>> +     t = map_sysmem(*current, len);
>>       memset(t, 0, sizeof(struct smbios_type3));
>>       fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle);
>>       t->manufacturer = smbios_add_string(t->eos, CONFIG_SMBIOS_MANUFACTURER);
>> @@ -163,6 +171,7 @@ static int smbios_write_type3(ulong *current, int handle)
>>
>>       len = t->length + smbios_string_table_len(t->eos);
>>       *current += len;
>> +     unmap_sysmem(t);
>>
>>       return len;
>>  }
>> @@ -201,9 +210,10 @@ static void smbios_write_type4_dm(struct smbios_type4 *t)
>>
>>  static int smbios_write_type4(ulong *current, int handle)
>>  {
>> -     struct smbios_type4 *t = (struct smbios_type4 *)*current;
>> +     struct smbios_type4 *t;
>>       int len = sizeof(struct smbios_type4);
>>
>> +     t = map_sysmem(*current, len);
>>       memset(t, 0, sizeof(struct smbios_type4));
>>       fill_smbios_header(t, SMBIOS_PROCESSOR_INFORMATION, len, handle);
>>       t->processor_type = SMBIOS_PROCESSOR_TYPE_CENTRAL;
>> @@ -217,32 +227,37 @@ static int smbios_write_type4(ulong *current, int handle)
>>
>>       len = t->length + smbios_string_table_len(t->eos);
>>       *current += len;
>> +     unmap_sysmem(t);
>>
>>       return len;
>>  }
>>
>>  static int smbios_write_type32(ulong *current, int handle)
>>  {
>> -     struct smbios_type32 *t = (struct smbios_type32 *)*current;
>> +     struct smbios_type32 *t;
>>       int len = sizeof(struct smbios_type32);
>>
>> +     t = map_sysmem(*current, len);
>>       memset(t, 0, sizeof(struct smbios_type32));
>>       fill_smbios_header(t, SMBIOS_SYSTEM_BOOT_INFORMATION, len, handle);
>>
>>       *current += len;
>> +     unmap_sysmem(t);
>>
>>       return len;
>>  }
>>
>>  static int smbios_write_type127(ulong *current, int handle)
>>  {
>> -     struct smbios_type127 *t = (struct smbios_type127 *)*current;
>> +     struct smbios_type127 *t;
>>       int len = sizeof(struct smbios_type127);
>>
>> +     t = map_sysmem(*current, len);
>>       memset(t, 0, sizeof(struct smbios_type127));
>>       fill_smbios_header(t, SMBIOS_END_OF_TABLE, len, handle);
>>
>>       *current += len;
>> +     unmap_sysmem(t);
>>
>>       return len;
>>  }
>> @@ -271,7 +286,7 @@ ulong write_smbios_table(ulong addr)
>>       /* 16 byte align the table address */
>>       addr = ALIGN(addr, 16);
>>
>> -     se = (struct smbios_entry *)(uintptr_t)addr;
>> +     se = map_sysmem(addr, sizeof(struct smbios_entry));
>>       memset(se, 0, sizeof(struct smbios_entry));
>>
>>       addr += sizeof(struct smbios_entry);
>> @@ -280,9 +295,13 @@ ulong write_smbios_table(ulong addr)
>>
>>       /* populate minimum required tables */
>>       for (i = 0; i < ARRAY_SIZE(smbios_write_funcs); i++) {
>> -             int tmp = smbios_write_funcs[i]((ulong *)&addr, handle++);
>> +             ulong *ptr = map_sysmem(addr, 0);
>
> This replaces &addr by addr and breaks booting on ARM64.
>
> Did you mean:
>                 ulong *ptr = map_sysmem((phys_addr_t)&addr, 0);

No, actually I don't think this is really needed at all since the
version is done by the functions it calls.

>
> The coding would get much easier to maintain if the map_sysmem()
> function would be eliminated from U-Boot.
>
> Why don't you simply use the address range that mmap() has provided?
> This would avoid double book keeping.

It is on my mind. But the address is used in U-Boot (e.g. in scripts).
Every other board has a defined and known SDRAM start address, and I
think it is best that sandbox has this too. It is also a good marker
of the conversion between an address and a pointer. IMO using a cast
is a bit ad-hoc.

Regards,
Simon