[U-Boot] [PATCH v14 0/4] efi_loader: Code refactoring and improvement

[U-Boot] [PATCH v14 0/4] efi_loader: Code refactoring and improvement

[Simon Glass]

This collects the patches previously sent to break up the very large
functions in efi_loader into smaller pieces. Now that the other sandbox
stuff is applied, perhaps it is time to apply these patches.

This also adds a few new patches to fix more recent breakages.
Unfortunately we still cannot enable the efi loader tests since one of
the tests fails. Thus we should expect additional failures to appear
until that is resolved.

Changes in v14:
- Fix condition for invalid pointer
- Go back to the horrible long variable names
- Hopefully correct error paths in do_bootefi_exec()

Changes in v13:
- Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
- Drop patches previously applied
- Rebase to efi/efi-next
- Update code to deal with the struct_table_address member

Changes in v12:
- Rename image to image_prot

Changes in v11:
- Drop patches previously applied
- Fix the EFI code that has since been added and relies on broken behaviour

Changes in v9:
- Add comments to bootefi_test_prepare() about the memset()s

Changes in v7:
- Drop patch "efi: Init the 'rows' and 'cols' variables"
- Drop patches previous applied

Changes in v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()
- Introduce load_options_path to specifyc U-Boot env var for load_options_path
- Rebase to master

Changes in v4:
- Rebase to master

Changes in v3:
- Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
- Add new patch to split out test init/uninit into functions
- Add patch to create a function to set up for running EFI code
- Drop incorrect map_sysmem() in write_smbios_table()

Simon Glass (4):
  sandbox: smbios: Update to support sandbox
  efi: Split out test init/uninit into functions
  efi: Create a function to set up for running EFI code
  efi: Rename bootefi_test_finish() to bootefi_run_finish()

 cmd/bootefi.c               | 120 +++++++++++++++++++++++++++---------
 lib/efi_loader/efi_smbios.c |  20 +++---
 lib/smbios.c                |  52 +++++++++++++---
 3 files changed, 147 insertions(+), 45 deletions(-)

-- 
2.19.1.1215.g8438c0b245-goog

[U-Boot] [PATCH v14 1/4] 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 v14:
- Fix condition for invalid pointer

Changes in v13:
- Update code to deal with the struct_table_address member

Changes in v12: None
Changes in v11:
- Fix the EFI code that has since been added and relies on broken behaviour

Changes in v9: None
Changes in v7: None
Changes in v5: None
Changes in v4: None
Changes in v3:
- Drop incorrect map_sysmem() in write_smbios_table()

 lib/efi_loader/efi_smbios.c | 20 +++++++++-----
 lib/smbios.c                | 52 ++++++++++++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 38e42fa2432..a81488495e2 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -7,6 +7,7 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <mapmem.h>
 #include <smbios.h>
 
 static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
@@ -19,17 +20,19 @@ static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
 efi_status_t efi_smbios_register(void)
 {
 	/* Map within the low 32 bits, to allow for 32bit SMBIOS tables */
-	u64 dmi = U32_MAX;
+	u64 dmi_addr = U32_MAX;
 	efi_status_t ret;
+	void *dmi;
 
 	/* Reserve 4kiB page for SMBIOS */
 	ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
-				 EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
+				 EFI_RUNTIME_SERVICES_DATA, 1, &dmi_addr);
 
 	if (ret != EFI_SUCCESS) {
 		/* Could not find space in lowmem, use highmem instead */
 		ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
-					 EFI_RUNTIME_SERVICES_DATA, 1, &dmi);
+					 EFI_RUNTIME_SERVICES_DATA, 1,
+					 &dmi_addr);
 
 		if (ret != EFI_SUCCESS)
 			return ret;
@@ -39,11 +42,14 @@ efi_status_t efi_smbios_register(void)
 	 * Generate SMBIOS tables - we know that efi_allocate_pages() returns
 	 * a 4k-aligned address, so it is safe to assume that
 	 * write_smbios_table() will write the table at that address.
+	 *
+	 * Note that on sandbox, efi_allocate_pages() unfortunately returns a
+	 * pointer even though it uses a uint64_t type. Convert it.
 	 */
-	assert(!(dmi & 0xf));
-	write_smbios_table(dmi);
+	assert(!(dmi_addr & 0xf));
+	dmi = (void *)(uintptr_t)dmi_addr;
+	write_smbios_table(map_to_sysmem(dmi));
 
 	/* And expose them to our EFI payload */
-	return efi_install_configuration_table(&smbios_guid,
-					       (void *)(uintptr_t)dmi);
+	return efi_install_configuration_table(&smbios_guid, dmi);
 }
diff --git a/lib/smbios.c b/lib/smbios.c
index 326eb00230d..e8ee55c4aeb 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -6,6 +6,7 @@
  */
 
 #include <common.h>
+#include <mapmem.h>
 #include <smbios.h>
 #include <tables_csum.h>
 #include <version.h>
@@ -72,9 +73,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");
@@ -101,16 +103,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);
@@ -122,15 +126,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);
@@ -140,15 +146,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);
@@ -160,6 +168,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;
 }
@@ -198,9 +207,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;
@@ -214,32 +224,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;
 }
@@ -257,6 +272,7 @@ static smbios_write_type smbios_write_funcs[] = {
 ulong write_smbios_table(ulong addr)
 {
 	struct smbios_entry *se;
+	ulong table_addr;
 	ulong tables;
 	int len = 0;
 	int max_struct_size = 0;
@@ -268,7 +284,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);
@@ -290,7 +306,24 @@ ulong write_smbios_table(ulong addr)
 	se->max_struct_size = max_struct_size;
 	memcpy(se->intermediate_anchor, "_DMI_", 5);
 	se->struct_table_length = len;
-	se->struct_table_address = tables;
+
+	/*
+	 * We must use a pointer here so things work correctly on sandbox. The
+	 * user of this table is not aware of the mapping of addresses to
+	 * sandbox's DRAM buffer.
+	 */
+	table_addr = (ulong)map_sysmem(tables, 0);
+	if (sizeof(table_addr) > sizeof(u32) && table_addr > (ulong)UINT_MAX) {
+		/*
+		 * We need to put this >32-bit pointer into the table but the
+		 * field is only 32 bits wide.
+		 */
+		printf("WARNING: SMBIOS table_address overflow %llx\n",
+		       (unsigned long long)table_addr);
+		table_addr = 0;
+	}
+	se->struct_table_address = table_addr;
+
 	se->struct_count = handle;
 
 	/* calculate checksums */
@@ -298,6 +331,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.19.1.1215.g8438c0b245-goog

[U-Boot] [PATCH v14 2/4] efi: Split out test init/uninit into functions

[Simon Glass]

The functions in bootefi are very long because they mix high-level code
and control with the low-level implementation. To help with this, create
functions which handle preparing for running the test and cleaning up
afterwards.

Also shorten the awfully long variable names here.

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

Changes in v14:
- Go back to the horrible long variable names

Changes in v13:
- Rebase to efi/efi-next

Changes in v12:
- Rename image to image_prot

Changes in v11: None
Changes in v9:
- Add comments to bootefi_test_prepare() about the memset()s

Changes in v7: None
Changes in v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()

Changes in v4: None
Changes in v3:
- Add new patch to split out test init/uninit into functions

 cmd/bootefi.c | 81 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 16 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3e37805ea13..b633e956c23 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -453,6 +453,67 @@ exit:
 	return ret;
 }
 
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+/**
+ * bootefi_test_prepare() - prepare to run an EFI test
+ *
+ * This sets things up so we can call EFI functions. This involves preparing
+ * the 'gd' pointer and setting up the load ed image data structures.
+ *
+ * @image_objp: loaded_image_infop: Pointer to a struct which will hold the
+ *    loaded image object. This struct will be inited by this function before
+ *    use.
+ * @loaded_image_infop: Pointer to a struct which will hold the loaded image
+ *    info. This struct will be inited by this function before use.
+ * @path: File path to the test being run (often just the test name with a
+ *    backslash before it
+ * @test_func: Address of the test function that is being run
+ * @return 0 if OK, -ve on error
+ */
+static efi_status_t bootefi_test_prepare(
+		struct efi_loaded_image_obj **image_objp,
+		struct efi_loaded_image **loaded_image_infop,
+		const char *path,
+		ulong test_func)
+{
+	efi_status_t r;
+
+	/* Construct a dummy device path */
+	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+					      (uintptr_t)test_func,
+					      (uintptr_t)test_func);
+	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
+	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
+				   image_objp, loaded_image_infop);
+	if (r)
+		return r;
+	/*
+	 * gd lives in a fixed register which may get clobbered while we execute
+	 * the payload. So save it here and restore it on every callback entry
+	 */
+	efi_save_gd();
+
+	/* Transfer environment variable efi_selftest as load options */
+	set_load_options(*loaded_image_infop, "efi_selftest");
+
+	return 0;
+}
+
+/**
+ * bootefi_test_finish() - finish up after running an EFI test
+ *
+ * @image_obj: Pointer to a struct which holds the loaded image object
+ * @loaded_image_info: Pointer to a struct which holds the loaded image info
+ */
+static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
+				struct efi_loaded_image *loaded_image_info)
+{
+	efi_restore_gd();
+	free(loaded_image_info->load_options);
+	efi_delete_handle(&image_obj->header);
+}
+#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
+
 static int do_bootefi_bootmgr_exec(void)
 {
 	struct efi_device_path *device_path, *file_path;
@@ -528,26 +589,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		struct efi_loaded_image_obj *image_obj;
 		struct efi_loaded_image *loaded_image_info;
 
-		/* Construct a dummy device path. */
-		bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
-						      (uintptr_t)&efi_selftest,
-						      (uintptr_t)&efi_selftest);
-		bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest");
-
-		r = efi_setup_loaded_image(bootefi_device_path,
-					   bootefi_image_path, &image_obj,
-					   &loaded_image_info);
-		if (r != EFI_SUCCESS)
+		if (bootefi_test_prepare(&image_obj, &loaded_image_info,
+					 "\\selftest",
+					 (uintptr_t)&efi_selftest))
 			return CMD_RET_FAILURE;
 
-		efi_save_gd();
-		/* Transfer environment variable efi_selftest as load options */
-		set_load_options(loaded_image_info, "efi_selftest");
 		/* Execute the test */
 		r = efi_selftest(&image_obj->header, &systab);
-		efi_restore_gd();
-		free(loaded_image_info->load_options);
-		efi_delete_handle(&image_obj->header);
+		bootefi_test_finish(image_obj, loaded_image_info);
 		return r != EFI_SUCCESS;
 	} else
 #endif
-- 
2.19.1.1215.g8438c0b245-goog

[U-Boot] [PATCH v14 3/4] efi: Create a function to set up for running EFI code

[Simon Glass]

There is still duplicated code in efi_loader for tests and normal
operation.

Add a new bootefi_run_prepare() function which holds common code used to
set up U-Boot to run EFI code. Make use of this from the existing
bootefi_test_prepare() function, as well as do_bootefi_exec().

Also shorten a few variable names.

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

Changes in v14:
- Go back to the horrible long variable names

Changes in v13: None
Changes in v12: None
Changes in v11: None
Changes in v9: None
Changes in v7: None
Changes in v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()
- Introduce load_options_path to specifyc U-Boot env var for load_options_path

Changes in v4:
- Rebase to master

Changes in v3:
- Add patch to create a function to set up for running EFI code

 cmd/bootefi.c | 63 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 26 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index b633e956c23..ab7ada9f2d6 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -325,6 +325,31 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
 	return ret;
 }
 
+static efi_status_t bootefi_run_prepare(const char *load_options_path,
+		struct efi_device_path *device_path,
+		struct efi_device_path *image_path,
+		struct efi_loaded_image_obj **image_objp,
+		struct efi_loaded_image **loaded_image_infop)
+{
+	efi_status_t ret;
+
+	ret = efi_setup_loaded_image(device_path, image_path, image_objp,
+				     loaded_image_infop);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	/*
+	 * gd lives in a fixed register which may get clobbered while we execute
+	 * the payload. So save it here and restore it on every callback entry
+	 */
+	efi_save_gd();
+
+	/* Transfer environment variable as load options */
+	set_load_options(*loaded_image_infop, load_options_path);
+
+	return 0;
+}
+
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -374,13 +399,11 @@ static efi_status_t do_bootefi_exec(void *efi,
 		assert(device_path && image_path);
 	}
 
-	ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
-				     &loaded_image_info);
-	if (ret != EFI_SUCCESS)
-		goto exit;
+	ret = bootefi_run_prepare("bootargs", device_path, image_path,
+				  &image_obj, &loaded_image_info);
+	if (ret)
+		return ret;
 
-	/* Transfer environment variable bootargs as load options */
-	set_load_options(loaded_image_info, "bootargs");
 	/* Load the EFI payload */
 	entry = efi_load_pe(image_obj, efi, loaded_image_info);
 	if (!entry) {
@@ -468,35 +491,23 @@ exit:
  * @path: File path to the test being run (often just the test name with a
  *    backslash before it
  * @test_func: Address of the test function that is being run
+ * @load_options_path: U-Boot environment variable to use as load options
  * @return 0 if OK, -ve on error
  */
 static efi_status_t bootefi_test_prepare(
 		struct efi_loaded_image_obj **image_objp,
-		struct efi_loaded_image **loaded_image_infop,
-		const char *path,
-		ulong test_func)
+		struct efi_loaded_image **loaded_image_infop, const char *path,
+		ulong test_func, const char *load_options_path)
 {
-	efi_status_t r;
-
 	/* Construct a dummy device path */
 	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
 					      (uintptr_t)test_func,
 					      (uintptr_t)test_func);
 	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
-	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
-				   image_objp, loaded_image_infop);
-	if (r)
-		return r;
-	/*
-	 * gd lives in a fixed register which may get clobbered while we execute
-	 * the payload. So save it here and restore it on every callback entry
-	 */
-	efi_save_gd();
-
-	/* Transfer environment variable efi_selftest as load options */
-	set_load_options(*loaded_image_infop, "efi_selftest");
 
-	return 0;
+	return bootefi_run_prepare(load_options_path, bootefi_device_path,
+				   bootefi_image_path, image_objp,
+				   loaded_image_infop);
 }
 
 /**
@@ -590,8 +601,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		struct efi_loaded_image *loaded_image_info;
 
 		if (bootefi_test_prepare(&image_obj, &loaded_image_info,
-					 "\\selftest",
-					 (uintptr_t)&efi_selftest))
+					 "\\selftest", (uintptr_t)&efi_selftest,
+					 "efi_selftest"))
 			return CMD_RET_FAILURE;
 
 		/* Execute the test */
-- 
2.19.1.1215.g8438c0b245-goog

[U-Boot] [PATCH v14 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()

[Simon Glass]

This function can be used from do_bootefi_exec() so that we use mostly the
same code for a normal EFI application and an EFI test.

Rename the function and use it in both places.

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

Changes in v14:
- Go back to the horrible long variable names
- Hopefully correct error paths in do_bootefi_exec()

Changes in v13:
- Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
- Drop patches previously applied

Changes in v12: None
Changes in v11:
- Drop patches previously applied

Changes in v9: None
Changes in v7:
- Drop patch "efi: Init the 'rows' and 'cols' variables"
- Drop patches previous applied

Changes in v5:
- Rebase to master

Changes in v4:
- Rebase to master

Changes in v3:
- Add new patch to rename bootefi_test_finish() to bootefi_run_finish()

 cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index ab7ada9f2d6..a627f689f95 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -350,6 +350,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
 	return 0;
 }
 
+/**
+ * bootefi_run_finish() - finish up after running an EFI test
+ *
+ * @loaded_image_info: Pointer to a struct which holds the loaded image info
+ * @image_objj: Pointer to a struct which holds the loaded image object
+ */
+static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
+			       struct efi_loaded_image *loaded_image_info)
+{
+	efi_restore_gd();
+	free(loaded_image_info->load_options);
+	efi_delete_handle(&image_obj->header);
+}
+
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -390,11 +404,11 @@ static efi_status_t do_bootefi_exec(void *efi,
 		 */
 		ret = efi_create_handle(&mem_handle);
 		if (ret != EFI_SUCCESS)
-			goto exit;
+			return ret;
 		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
 				       device_path);
 		if (ret != EFI_SUCCESS)
-			goto exit;
+			goto err_add_protocol;
 	} else {
 		assert(device_path && image_path);
 	}
@@ -402,13 +416,13 @@ static efi_status_t do_bootefi_exec(void *efi,
 	ret = bootefi_run_prepare("bootargs", device_path, image_path,
 				  &image_obj, &loaded_image_info);
 	if (ret)
-		return ret;
+		goto err_prepare;
 
 	/* Load the EFI payload */
 	entry = efi_load_pe(image_obj, efi, loaded_image_info);
 	if (!entry) {
 		ret = EFI_LOAD_ERROR;
-		goto exit;
+		goto err_prepare;
 	}
 
 	if (memdp) {
@@ -428,7 +442,7 @@ static efi_status_t do_bootefi_exec(void *efi,
 
 	if (setjmp(&image_obj->exit_jmp)) {
 		ret = image_obj->exit_status;
-		goto exit;
+		goto err_prepare;
 	}
 
 #ifdef CONFIG_ARM64
@@ -466,10 +480,11 @@ static efi_status_t do_bootefi_exec(void *efi,
 
 	ret = efi_do_enter(&image_obj->header, &systab, entry);
 
-exit:
+err_prepare:
 	/* image has returned, loaded-image obj goes *poof*: */
-	if (image_obj)
-		efi_delete_handle(&image_obj->header);
+	bootefi_run_finish(image_obj, loaded_image_info);
+
+err_add_protocol:
 	if (mem_handle)
 		efi_delete_handle(mem_handle);
 
@@ -510,19 +525,6 @@ static efi_status_t bootefi_test_prepare(
 				   loaded_image_infop);
 }
 
-/**
- * bootefi_test_finish() - finish up after running an EFI test
- *
- * @image_obj: Pointer to a struct which holds the loaded image object
- * @loaded_image_info: Pointer to a struct which holds the loaded image info
- */
-static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
-				struct efi_loaded_image *loaded_image_info)
-{
-	efi_restore_gd();
-	free(loaded_image_info->load_options);
-	efi_delete_handle(&image_obj->header);
-}
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
 static int do_bootefi_bootmgr_exec(void)
@@ -607,7 +609,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 		/* Execute the test */
 		r = efi_selftest(&image_obj->header, &systab);
-		bootefi_test_finish(image_obj, loaded_image_info);
+		bootefi_run_finish(image_obj, loaded_image_info);
 		return r != EFI_SUCCESS;
 	} else
 #endif
-- 
2.19.1.1215.g8438c0b245-goog

[U-Boot] [PATCH v14 0/4] efi_loader: Code refactoring and improvement

[Simon Glass]

Hi Alex,

On Wed, 14 Nov 2018 at 16:11, Simon Glass <sjg@chromium.org> wrote:
>
> This collects the patches previously sent to break up the very large
> functions in efi_loader into smaller pieces. Now that the other sandbox
> stuff is applied, perhaps it is time to apply these patches.
>
> This also adds a few new patches to fix more recent breakages.
> Unfortunately we still cannot enable the efi loader tests since one of
> the tests fails. Thus we should expect additional failures to appear
> until that is resolved.
>
> Changes in v14:
> - Fix condition for invalid pointer
> - Go back to the horrible long variable names
> - Hopefully correct error paths in do_bootefi_exec()

Any thoughts on this series please?

Regards,
Simon

[U-Boot] [PATCH v14 0/4] efi_loader: Code refactoring and improvement

[Simon Glass]

Hi Heinrich,

On Sun, 18 Nov 2018 at 08:20, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Alex,
>
> On Wed, 14 Nov 2018 at 16:11, Simon Glass <sjg@chromium.org> wrote:
> >
> > This collects the patches previously sent to break up the very large
> > functions in efi_loader into smaller pieces. Now that the other sandbox
> > stuff is applied, perhaps it is time to apply these patches.
> >
> > This also adds a few new patches to fix more recent breakages.
> > Unfortunately we still cannot enable the efi loader tests since one of
> > the tests fails. Thus we should expect additional failures to appear
> > until that is resolved.
> >
> > Changes in v14:
> > - Fix condition for invalid pointer
> > - Go back to the horrible long variable names
> > - Hopefully correct error paths in do_bootefi_exec()
>
> Any thoughts on this series please?

Could you please take another look at this? I would very much like to
put it to bed.

Regards,
Simon

[U-Boot] [PATCH v14 2/4] efi: Split out test init/uninit into functions

[Heinrich Schuchardt]

On 11/15/18 12:11 AM, Simon Glass wrote:
> The functions in bootefi are very long because they mix high-level code
> and control with the low-level implementation. To help with this, create
> functions which handle preparing for running the test and cleaning up
> afterwards.
> 
> Also shorten the awfully long variable names here.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v14:
> - Go back to the horrible long variable names
> 
> Changes in v13:
> - Rebase to efi/efi-next
> 
> Changes in v12:
> - Rename image to image_prot
> 
> Changes in v11: None
> Changes in v9:
> - Add comments to bootefi_test_prepare() about the memset()s
> 
> Changes in v7: None
> Changes in v5:
> - Drop call to efi_init_obj_list() which is now done in do_bootefi()
> 
> Changes in v4: None
> Changes in v3:
> - Add new patch to split out test init/uninit into functions
> 
>  cmd/bootefi.c | 81 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3e37805ea13..b633e956c23 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -453,6 +453,67 @@ exit:
>  	return ret;
>  }
>  
> +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> +/**
> + * bootefi_test_prepare() - prepare to run an EFI test
> + *
> + * This sets things up so we can call EFI functions. This involves preparing
> + * the 'gd' pointer and setting up the load ed image data structures.
> + *
> + * @image_objp: loaded_image_infop: Pointer to a struct which will hold the
> + *    loaded image object. This struct will be inited by this function before
> + *    use.
> + * @loaded_image_infop: Pointer to a struct which will hold the loaded image
> + *    info. This struct will be inited by this function before use.
> + * @path: File path to the test being run (often just the test name with a
> + *    backslash before it
> + * @test_func: Address of the test function that is being run
> + * @return 0 if OK, -ve on error
> + */
> +static efi_status_t bootefi_test_prepare(

CHECK: Lines should not end with a '('
#43: FILE: cmd/bootefi.c:474:
+static efi_status_t bootefi_test_prepare(

So the parenthesis has to go onto the next line.

> +		struct efi_loaded_image_obj **image_objp,

		(struct efi_loaded_image_obj **image_objp,

> +		struct efi_loaded_image **loaded_image_infop,
> +		const char *path,
> +		ulong test_func)
> +{
> +	efi_status_t r;
> +
> +	/* Construct a dummy device path */
> +	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					      (uintptr_t)test_func,
> +					      (uintptr_t)test_func);

We have to check the return value. It may be NULL.

> +	bootefi_image_path = efi_dp_from_file(NULL, 0, path);

Same here.

> +	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
> +				   image_objp, loaded_image_infop);
> +	if (r)
> +		return r;
> +	/*
> +	 * gd lives in a fixed register which may get clobbered while we execute
> +	 * the payload. So save it here and restore it on every callback entry
> +	 */
> +	efi_save_gd();

We do this already in efi_init_obj_list().

Best regards

Heinrich

> +
> +	/* Transfer environment variable efi_selftest as load options */
> +	set_load_options(*loaded_image_infop, "efi_selftest");
> +
> +	return 0;
> +}
> +
> +/**
> + * bootefi_test_finish() - finish up after running an EFI test
> + *
> + * @image_obj: Pointer to a struct which holds the loaded image object
> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> + */
> +static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
> +				struct efi_loaded_image *loaded_image_info)
> +{
> +	efi_restore_gd();
> +	free(loaded_image_info->load_options);
> +	efi_delete_handle(&image_obj->header);
> +}
> +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> +
>  static int do_bootefi_bootmgr_exec(void)
>  {
>  	struct efi_device_path *device_path, *file_path;
> @@ -528,26 +589,14 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		struct efi_loaded_image_obj *image_obj;
>  		struct efi_loaded_image *loaded_image_info;
>  
> -		/* Construct a dummy device path. */
> -		bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> -						      (uintptr_t)&efi_selftest,
> -						      (uintptr_t)&efi_selftest);
> -		bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest");
> -
> -		r = efi_setup_loaded_image(bootefi_device_path,
> -					   bootefi_image_path, &image_obj,
> -					   &loaded_image_info);
> -		if (r != EFI_SUCCESS)
> +		if (bootefi_test_prepare(&image_obj, &loaded_image_info,
> +					 "\\selftest",
> +					 (uintptr_t)&efi_selftest))
>  			return CMD_RET_FAILURE;
>  
> -		efi_save_gd();
> -		/* Transfer environment variable efi_selftest as load options */
> -		set_load_options(loaded_image_info, "efi_selftest");
>  		/* Execute the test */
>  		r = efi_selftest(&image_obj->header, &systab);
> -		efi_restore_gd();
> -		free(loaded_image_info->load_options);
> -		efi_delete_handle(&image_obj->header);
> +		bootefi_test_finish(image_obj, loaded_image_info);
>  		return r != EFI_SUCCESS;
>  	} else
>  #endif
> 

[U-Boot] [PATCH v14 3/4] efi: Create a function to set up for running EFI code

[Heinrich Schuchardt]

On 11/15/18 12:11 AM, Simon Glass wrote:
> There is still duplicated code in efi_loader for tests and normal
> operation.
> 
> Add a new bootefi_run_prepare() function which holds common code used to
> set up U-Boot to run EFI code. Make use of this from the existing
> bootefi_test_prepare() function, as well as do_bootefi_exec().
> 
> Also shorten a few variable names.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v14:
> - Go back to the horrible long variable names
> 
> Changes in v13: None
> Changes in v12: None
> Changes in v11: None
> Changes in v9: None
> Changes in v7: None
> Changes in v5:
> - Drop call to efi_init_obj_list() which is now done in do_bootefi()
> - Introduce load_options_path to specifyc U-Boot env var for load_options_path
> 
> Changes in v4:
> - Rebase to master
> 
> Changes in v3:
> - Add patch to create a function to set up for running EFI code
> 
>  cmd/bootefi.c | 63 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index b633e956c23..ab7ada9f2d6 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -325,6 +325,31 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
>  	return ret;
>  }
>  
> +static efi_status_t bootefi_run_prepare(const char *load_options_path,
> +		struct efi_device_path *device_path,
> +		struct efi_device_path *image_path,
> +		struct efi_loaded_image_obj **image_objp,
> +		struct efi_loaded_image **loaded_image_infop)
> +{
> +	efi_status_t ret;
> +
> +	ret = efi_setup_loaded_image(device_path, image_path, image_objp,
> +				     loaded_image_infop);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	/*
> +	 * gd lives in a fixed register which may get clobbered while we execute
> +	 * the payload. So save it here and restore it on every callback entry
> +	 */
> +	efi_save_gd();
> +
> +	/* Transfer environment variable as load options */
> +	set_load_options(*loaded_image_infop, load_options_path);
> +
> +	return 0;
> +}
> +
>  /**
>   * do_bootefi_exec() - execute EFI binary
>   *
> @@ -374,13 +399,11 @@ static efi_status_t do_bootefi_exec(void *efi,
>  		assert(device_path && image_path);
>  	}
>  
> -	ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> -				     &loaded_image_info);
> -	if (ret != EFI_SUCCESS)
> -		goto exit;
> +	ret = bootefi_run_prepare("bootargs", device_path, image_path,
> +				  &image_obj, &loaded_image_info);
> +	if (ret)
> +		return ret;
>  
> -	/* Transfer environment variable bootargs as load options */
> -	set_load_options(loaded_image_info, "bootargs");
>  	/* Load the EFI payload */
>  	entry = efi_load_pe(image_obj, efi, loaded_image_info);
>  	if (!entry) {
> @@ -468,35 +491,23 @@ exit:
>   * @path: File path to the test being run (often just the test name with a
>   *    backslash before it
>   * @test_func: Address of the test function that is being run
> + * @load_options_path: U-Boot environment variable to use as load options
>   * @return 0 if OK, -ve on error
>   */
>  static efi_status_t bootefi_test_prepare(

CHECK: Alignment should match open parenthesis
#29: FILE: cmd/bootefi.c:330:
+static efi_status_t bootefi_run_prepare(const char *load_options_path,
+               struct efi_device_path *device_path,

>  		struct efi_loaded_image_obj **image_objp,
> -		struct efi_loaded_image **loaded_image_infop,
> -		const char *path,
> -		ulong test_func)
> +		struct efi_loaded_image **loaded_image_infop, const char *path,
> +		ulong test_func, const char *load_options_path)
>  {
> -	efi_status_t r;
> -
>  	/* Construct a dummy device path */
>  	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>  					      (uintptr_t)test_func,
>  					      (uintptr_t)test_func);
>  	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
> -	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
> -				   image_objp, loaded_image_infop);
> -	if (r)
> -		return r;
> -	/*
> -	 * gd lives in a fixed register which may get clobbered while we execute
> -	 * the payload. So save it here and restore it on every callback entry
> -	 */
> -	efi_save_gd();
> -
> -	/* Transfer environment variable efi_selftest as load options */
> -	set_load_options(*loaded_image_infop, "efi_selftest");
>  
> -	return 0;
> +	return bootefi_run_prepare(load_options_path, bootefi_device_path,
> +				   bootefi_image_path, image_objp,
> +				   loaded_image_infop);
>  }
>  
>  /**
> @@ -590,8 +601,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		struct efi_loaded_image *loaded_image_info;
>  
>  		if (bootefi_test_prepare(&image_obj, &loaded_image_info,
> -					 "\\selftest",
> -					 (uintptr_t)&efi_selftest))
> +					 "\\selftest", (uintptr_t)&efi_selftest,
> +					 "efi_selftest"))
>  			return CMD_RET_FAILURE;
>  
>  		/* Execute the test */
> 

[U-Boot] [PATCH v14 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()

[Heinrich Schuchardt]

On 11/15/18 12:11 AM, Simon Glass wrote:
> This function can be used from do_bootefi_exec() so that we use mostly the
> same code for a normal EFI application and an EFI test.
> 
> Rename the function and use it in both places.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v14:
> - Go back to the horrible long variable names
> - Hopefully correct error paths in do_bootefi_exec()
> 
> Changes in v13:
> - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
> - Drop patches previously applied
> 
> Changes in v12: None
> Changes in v11:
> - Drop patches previously applied
> 
> Changes in v9: None
> Changes in v7:
> - Drop patch "efi: Init the 'rows' and 'cols' variables"
> - Drop patches previous applied
> 
> Changes in v5:
> - Rebase to master
> 
> Changes in v4:
> - Rebase to master
> 
> Changes in v3:
> - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
> 
>  cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index ab7ada9f2d6..a627f689f95 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -350,6 +350,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
>  	return 0;
>  }
>  
> +/**
> + * bootefi_run_finish() - finish up after running an EFI test
> + *
> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> + * @image_objj: Pointer to a struct which holds the loaded image object
> + */
> +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> +			       struct efi_loaded_image *loaded_image_info)
> +{
> +	efi_restore_gd();
> +	free(loaded_image_info->load_options);
> +	efi_delete_handle(&image_obj->header);
> +}
> +
>  /**
>   * do_bootefi_exec() - execute EFI binary
>   *
> @@ -390,11 +404,11 @@ static efi_status_t do_bootefi_exec(void *efi,
>  		 */
>  		ret = efi_create_handle(&mem_handle);
>  		if (ret != EFI_SUCCESS)
> -			goto exit;

You are leaking a device path.

Best regards

Heinrich

> +			return ret;
>  		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>  				       device_path);
>  		if (ret != EFI_SUCCESS)
> -			goto exit;
> +			goto err_add_protocol;
>  	} else {
>  		assert(device_path && image_path);
>  	}
> @@ -402,13 +416,13 @@ static efi_status_t do_bootefi_exec(void *efi,
>  	ret = bootefi_run_prepare("bootargs", device_path, image_path,
>  				  &image_obj, &loaded_image_info);
>  	if (ret)
> -		return ret;
> +		goto err_prepare;
>  
>  	/* Load the EFI payload */
>  	entry = efi_load_pe(image_obj, efi, loaded_image_info);
>  	if (!entry) {
>  		ret = EFI_LOAD_ERROR;
> -		goto exit;
> +		goto err_prepare;
>  	}
>  
>  	if (memdp) {
> @@ -428,7 +442,7 @@ static efi_status_t do_bootefi_exec(void *efi,
>  
>  	if (setjmp(&image_obj->exit_jmp)) {
>  		ret = image_obj->exit_status;
> -		goto exit;
> +		goto err_prepare;
>  	}
>  
>  #ifdef CONFIG_ARM64
> @@ -466,10 +480,11 @@ static efi_status_t do_bootefi_exec(void *efi,
>  
>  	ret = efi_do_enter(&image_obj->header, &systab, entry);
>  
> -exit:
> +err_prepare:
>  	/* image has returned, loaded-image obj goes *poof*: */
> -	if (image_obj)
> -		efi_delete_handle(&image_obj->header);
> +	bootefi_run_finish(image_obj, loaded_image_info);
> +
> +err_add_protocol:
>  	if (mem_handle)
>  		efi_delete_handle(mem_handle);
>  
> @@ -510,19 +525,6 @@ static efi_status_t bootefi_test_prepare(
>  				   loaded_image_infop);
>  }
>  
> -/**
> - * bootefi_test_finish() - finish up after running an EFI test
> - *
> - * @image_obj: Pointer to a struct which holds the loaded image object
> - * @loaded_image_info: Pointer to a struct which holds the loaded image info
> - */
> -static void bootefi_test_finish(struct efi_loaded_image_obj *image_obj,
> -				struct efi_loaded_image *loaded_image_info)
> -{
> -	efi_restore_gd();
> -	free(loaded_image_info->load_options);
> -	efi_delete_handle(&image_obj->header);
> -}
>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>  
>  static int do_bootefi_bootmgr_exec(void)
> @@ -607,7 +609,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  
>  		/* Execute the test */
>  		r = efi_selftest(&image_obj->header, &systab);
> -		bootefi_test_finish(image_obj, loaded_image_info);
> +		bootefi_run_finish(image_obj, loaded_image_info);
>  		return r != EFI_SUCCESS;
>  	} else
>  #endif
> 

[U-Boot] [PATCH v14 0/4] efi_loader: Code refactoring and improvement

[Heinrich Schuchardt]

On 11/19/18 2:49 AM, Simon Glass wrote:
> Hi Heinrich,
> 
> On Sun, 18 Nov 2018 at 08:20, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Alex,
>>
>> On Wed, 14 Nov 2018 at 16:11, Simon Glass <sjg@chromium.org> wrote:
>>>
>>> This collects the patches previously sent to break up the very large
>>> functions in efi_loader into smaller pieces. Now that the other sandbox
>>> stuff is applied, perhaps it is time to apply these patches.
>>>
>>> This also adds a few new patches to fix more recent breakages.
>>> Unfortunately we still cannot enable the efi loader tests since one of
>>> the tests fails. Thus we should expect additional failures to appear
>>> until that is resolved.
>>>
>>> Changes in v14:
>>> - Fix condition for invalid pointer
>>> - Go back to the horrible long variable names
>>> - Hopefully correct error paths in do_bootefi_exec()
>>
>> Any thoughts on this series please?
> 
> Could you please take another look at this? I would very much like to
> put it to bed.
> 
> Regards,
> Simon
> 

Refactoring into shorter functions makes sense to me.

The errors I mentioned in the review comments for the individual patches
were not introduced by you but we should not simply copy but correct them.

Best regards

Heinrich

[U-Boot] [PATCH v14 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()

[Simon Glass]

Hi Heinrich,

On Mon, 19 Nov 2018 at 12:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/15/18 12:11 AM, Simon Glass wrote:
> > This function can be used from do_bootefi_exec() so that we use mostly the
> > same code for a normal EFI application and an EFI test.
> >
> > Rename the function and use it in both places.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v14:
> > - Go back to the horrible long variable names
> > - Hopefully correct error paths in do_bootefi_exec()
> >
> > Changes in v13:
> > - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
> > - Drop patches previously applied
> >
> > Changes in v12: None
> > Changes in v11:
> > - Drop patches previously applied
> >
> > Changes in v9: None
> > Changes in v7:
> > - Drop patch "efi: Init the 'rows' and 'cols' variables"
> > - Drop patches previous applied
> >
> > Changes in v5:
> > - Rebase to master
> >
> > Changes in v4:
> > - Rebase to master
> >
> > Changes in v3:
> > - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
> >
> >  cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
> >  1 file changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index ab7ada9f2d6..a627f689f95 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -350,6 +350,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
> >       return 0;
> >  }
> >
> > +/**
> > + * bootefi_run_finish() - finish up after running an EFI test
> > + *
> > + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> > + * @image_objj: Pointer to a struct which holds the loaded image object
> > + */
> > +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> > +                            struct efi_loaded_image *loaded_image_info)
> > +{
> > +     efi_restore_gd();
> > +     free(loaded_image_info->load_options);
> > +     efi_delete_handle(&image_obj->header);
> > +}
> > +
> >  /**
> >   * do_bootefi_exec() - execute EFI binary
> >   *
> > @@ -390,11 +404,11 @@ static efi_status_t do_bootefi_exec(void *efi,
> >                */
> >               ret = efi_create_handle(&mem_handle);
> >               if (ret != EFI_SUCCESS)
> > -                     goto exit;
>
> You are leaking a device path.

I cannot see anywhere where these are freed. Are you sure this is supported?

I'll add a comment for now. In any case, this matches the previous code.

Regards,
Simon

[U-Boot] [PATCH v14 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()

[Heinrich Schuchardt]

On 11/22/18 9:50 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 19 Nov 2018 at 12:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 11/15/18 12:11 AM, Simon Glass wrote:
>>> This function can be used from do_bootefi_exec() so that we use mostly the
>>> same code for a normal EFI application and an EFI test.
>>>
>>> Rename the function and use it in both places.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v14:
>>> - Go back to the horrible long variable names
>>> - Hopefully correct error paths in do_bootefi_exec()
>>>
>>> Changes in v13:
>>> - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
>>> - Drop patches previously applied
>>>
>>> Changes in v12: None
>>> Changes in v11:
>>> - Drop patches previously applied
>>>
>>> Changes in v9: None
>>> Changes in v7:
>>> - Drop patch "efi: Init the 'rows' and 'cols' variables"
>>> - Drop patches previous applied
>>>
>>> Changes in v5:
>>> - Rebase to master
>>>
>>> Changes in v4:
>>> - Rebase to master
>>>
>>> Changes in v3:
>>> - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
>>>
>>>  cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
>>>  1 file changed, 24 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index ab7ada9f2d6..a627f689f95 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -350,6 +350,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
>>>       return 0;
>>>  }
>>>
>>> +/**
>>> + * bootefi_run_finish() - finish up after running an EFI test
>>> + *
>>> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
>>> + * @image_objj: Pointer to a struct which holds the loaded image object
>>> + */
>>> +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
>>> +                            struct efi_loaded_image *loaded_image_info)
>>> +{
>>> +     efi_restore_gd();
>>> +     free(loaded_image_info->load_options);
>>> +     efi_delete_handle(&image_obj->header);
>>> +}
>>> +
>>>  /**
>>>   * do_bootefi_exec() - execute EFI binary
>>>   *
>>> @@ -390,11 +404,11 @@ static efi_status_t do_bootefi_exec(void *efi,
>>>                */
>>>               ret = efi_create_handle(&mem_handle);
>>>               if (ret != EFI_SUCCESS)
>>> -                     goto exit;
>>
>> You are leaking a device path.
> 
> I cannot see anywhere where these are freed. Are you sure this is supported?

According to the UEFI standard handles are deleted when the last
protocol is removed. See efi_uninstall_protocol_interface(). I am aware
that our cleanup after running binaries is still incomplete.

In this function you could use efi_delete_handle() in the error handler.

Regards

Heinrich

> 
> I'll add a comment for now. In any case, this matches the previous code.
> 
> Regards,
> Simon
> 

[U-Boot] [PATCH v14 4/4] efi: Rename bootefi_test_finish() to bootefi_run_finish()

[Simon Glass]

Hi Heinrich,

On Fri, 23 Nov 2018 at 00:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/22/18 9:50 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Mon, 19 Nov 2018 at 12:41, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 11/15/18 12:11 AM, Simon Glass wrote:
> >>> This function can be used from do_bootefi_exec() so that we use mostly the
> >>> same code for a normal EFI application and an EFI test.
> >>>
> >>> Rename the function and use it in both places.
> >>>
> >>> Signed-off-by: Simon Glass <sjg@chromium.org>
> >>> ---
> >>>
> >>> Changes in v14:
> >>> - Go back to the horrible long variable names
> >>> - Hopefully correct error paths in do_bootefi_exec()
> >>>
> >>> Changes in v13:
> >>> - Drop 'efi_loader: Drop setup_ok' as we have an existing patch for that
> >>> - Drop patches previously applied
> >>>
> >>> Changes in v12: None
> >>> Changes in v11:
> >>> - Drop patches previously applied
> >>>
> >>> Changes in v9: None
> >>> Changes in v7:
> >>> - Drop patch "efi: Init the 'rows' and 'cols' variables"
> >>> - Drop patches previous applied
> >>>
> >>> Changes in v5:
> >>> - Rebase to master
> >>>
> >>> Changes in v4:
> >>> - Rebase to master
> >>>
> >>> Changes in v3:
> >>> - Add new patch to rename bootefi_test_finish() to bootefi_run_finish()
> >>>
> >>>  cmd/bootefi.c | 46 ++++++++++++++++++++++++----------------------
> >>>  1 file changed, 24 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index ab7ada9f2d6..a627f689f95 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -350,6 +350,20 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
> >>>       return 0;
> >>>  }
> >>>
> >>> +/**
> >>> + * bootefi_run_finish() - finish up after running an EFI test
> >>> + *
> >>> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> >>> + * @image_objj: Pointer to a struct which holds the loaded image object
> >>> + */
> >>> +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> >>> +                            struct efi_loaded_image *loaded_image_info)
> >>> +{
> >>> +     efi_restore_gd();
> >>> +     free(loaded_image_info->load_options);
> >>> +     efi_delete_handle(&image_obj->header);
> >>> +}
> >>> +
> >>>  /**
> >>>   * do_bootefi_exec() - execute EFI binary
> >>>   *
> >>> @@ -390,11 +404,11 @@ static efi_status_t do_bootefi_exec(void *efi,
> >>>                */
> >>>               ret = efi_create_handle(&mem_handle);
> >>>               if (ret != EFI_SUCCESS)
> >>> -                     goto exit;
> >>
> >> You are leaking a device path.
> >
> > I cannot see anywhere where these are freed. Are you sure this is supported?
>
> According to the UEFI standard handles are deleted when the last
> protocol is removed. See efi_uninstall_protocol_interface(). I am aware
> that our cleanup after running binaries is still incomplete.
>
> In this function you could use efi_delete_handle() in the error handler.

Actually the code is pretty impenetrable.

efi_delete_handle() takes an efi_handle_t which, confusingly, is a
pointer to an efi_object. Also we should not typedef structs in
U-Boot, particularly not with a different name.

In this case efi_create_handle() failed so we don't need to delete
that. We need to delete the efi_dp_from_mem() object. But the type of
that is struct efi_device_path, so I cannot call efi_delete_handle()
with it.

My suggestion is that if these patches land, you take a look at it and
try to figure out how to finish improving the error handling in this
function (which I started on patch v14 of this series). If I had my
way this code would be much, much simpler to read.

Regards,
Simon

[U-Boot] [PATCH v14 0/4] efi_loader: Code refactoring and improvement

[Simon Glass]

Hi Heinrich,

On Mon, 19 Nov 2018 at 12:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/19/18 2:49 AM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Sun, 18 Nov 2018 at 08:20, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi Alex,
> >>
> >> On Wed, 14 Nov 2018 at 16:11, Simon Glass <sjg@chromium.org> wrote:
> >>>
> >>> This collects the patches previously sent to break up the very large
> >>> functions in efi_loader into smaller pieces. Now that the other sandbox
> >>> stuff is applied, perhaps it is time to apply these patches.
> >>>
> >>> This also adds a few new patches to fix more recent breakages.
> >>> Unfortunately we still cannot enable the efi loader tests since one of
> >>> the tests fails. Thus we should expect additional failures to appear
> >>> until that is resolved.
> >>>
> >>> Changes in v14:
> >>> - Fix condition for invalid pointer
> >>> - Go back to the horrible long variable names
> >>> - Hopefully correct error paths in do_bootefi_exec()
> >>
> >> Any thoughts on this series please?
> >
> > Could you please take another look at this? I would very much like to
> > put it to bed.
> >
> > Regards,
> > Simon
> >
>
> Refactoring into shorter functions makes sense to me.
>
> The errors I mentioned in the review comments for the individual patches
> were not introduced by you but we should not simply copy but correct them.

I agree in principal, but within reason. It is easier in this case to
add patches afterwards.

Regards,
Simon