[U-Boot] [PATCH v2 0/4] efi_loader: rework loading and starting of images

[U-Boot] [PATCH v2 0/4] efi_loader: rework loading and starting of images

[Heinrich Schuchardt]

This patch series starts the necessary changes needed to correctly
implement the unloading of images in Exit().

If we want to properly unload images in Exit() the memory should always be
allocated in the same way. As we allocate memory when reading from file we
should do the same when the original image is in memory.

Up to now efi_load_pe() returns the entry point or NULL in case of an
error. This does not allow to return correct error codes from LoadImage().

Let efi_load_pe() return a status code and fill in the entry point in the
corresponding field of the image object.

Remove the duplicate code in efi_do_enter() and use efi_start_image() to
start the image invoked by the bootefi command.

Clean up bootefi_test_prepare().

Further patches will be needed:
- use LoadImage() in bootefi and bootmgr
- implement correct unloading of images in Exit()

Heinrich Schuchardt (4):
  efi_loader: LoadImage: always allocate new pages
  efi_loader: set entry point in efi_load_pe()
  efi_loader: use efi_start_image() for bootefi
  efi_loader: clean up bootefi_test_prepare()

 cmd/bootefi.c                     |  91 ++++++++++--------------
 include/efi_loader.h              |  10 ++-
 lib/efi_loader/efi_bootmgr.c      |   2 +-
 lib/efi_loader/efi_boottime.c     | 112 ++++++++++++++++++++----------
 lib/efi_loader/efi_image_loader.c |  37 +++++-----
 5 files changed, 141 insertions(+), 111 deletions(-)

-- 
2.20.1

[U-Boot] [PATCH v2 1/4] efi_loader: LoadImage: always allocate new pages

[Heinrich Schuchardt]

If we want to properly unload images in Exit() the memory should always be
allocated in the same way. As we allocate memory when reading from file we
should do the same when the original image is in memory.

A further patch will be needed to free the memory when Exit() is called.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	fix errors
	- missing parameters efi_size_in_pages(
	- inconsistent error handling
	- missing comments
---
 include/efi_loader.h          |   2 +-
 lib/efi_loader/efi_bootmgr.c  |   2 +-
 lib/efi_loader/efi_boottime.c | 100 ++++++++++++++++++++++++----------
 3 files changed, 72 insertions(+), 32 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9dd933dae7..6a98981ebb 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -397,7 +397,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
 				    struct efi_loaded_image_obj **handle_ptr,
 				    struct efi_loaded_image **info_ptr);
 efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
-				      void **buffer);
+				      void **buffer, efi_uintn_t *size);
 /* Print information about all loaded images */
 void efi_print_image_infos(void *pc);
 
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index a095df3f54..196116b547 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -150,7 +150,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
 		debug("%s: trying to load \"%ls\" from %pD\n",
 		      __func__, lo.label, lo.file_path);
 
-		ret = efi_load_image_from_path(lo.file_path, &image);
+		ret = efi_load_image_from_path(lo.file_path, &image, &size);
 
 		if (ret != EFI_SUCCESS)
 			goto error;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 6df20649c4..7abdcf6da6 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1593,50 +1593,72 @@ failure:
 
 /**
  * efi_load_image_from_path() - load an image using a file path
- * @file_path: the path of the image to load
- * @buffer:    buffer containing the loaded image
  *
- * Return: status code
+ * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
+ * callers obligation to update the memory type as needed.
+ *
+ * @file_path:	the path of the image to load
+ * @buffer:	buffer containing the loaded image
+ * @size:	size of the loaded image
+ * Return:	status code
  */
 efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
-				      void **buffer)
+				      void **buffer, efi_uintn_t *size)
 {
 	struct efi_file_info *info = NULL;
 	struct efi_file_handle *f;
 	static efi_status_t ret;
+	u64 addr;
 	efi_uintn_t bs;
 
+	/* In case of failure nothing is returned */
+	*buffer = NULL;
+	*size = 0;
+
+	/* Open file */
 	f = efi_file_from_path(file_path);
 	if (!f)
 		return EFI_DEVICE_ERROR;
 
+	/* Get file size */
 	bs = 0;
 	EFI_CALL(ret = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid,
 				  &bs, info));
-	if (ret == EFI_BUFFER_TOO_SMALL) {
-		info = malloc(bs);
-		EFI_CALL(ret = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid,
-					  &bs, info));
-	}
-	if (ret != EFI_SUCCESS)
+	if (ret != EFI_BUFFER_TOO_SMALL) {
+		ret =  EFI_DEVICE_ERROR;
 		goto error;
+	}
 
-	ret = efi_allocate_pool(EFI_LOADER_DATA, info->file_size, buffer);
-	if (ret)
+	info = malloc(bs);
+	EFI_CALL(ret = f->getinfo(f, (efi_guid_t *)&efi_file_info_guid, &bs,
+				  info));
+	if (ret != EFI_SUCCESS)
 		goto error;
 
+	/*
+	 * When reading the file we do not yet know if it contains an
+	 * application, a boottime driver, or a runtime driver. So here we
+	 * allocate a buffer as EFI_BOOT_SERVICES_DATA. The caller has to
+	 * update the reservation according to the image type.
+	 */
 	bs = info->file_size;
-	EFI_CALL(ret = f->read(f, &bs, *buffer));
-
-error:
-	free(info);
-	EFI_CALL(f->close(f));
-
+	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
+				 EFI_BOOT_SERVICES_DATA,
+				 efi_size_in_pages(bs), &addr);
 	if (ret != EFI_SUCCESS) {
-		efi_free_pool(*buffer);
-		*buffer = NULL;
+		ret = EFI_OUT_OF_RESOURCES;
+		goto error;
 	}
 
+	/* Read file */
+	EFI_CALL(ret = f->read(f, &bs, (void *)(uintptr_t)addr));
+	if (ret != EFI_SUCCESS)
+		efi_free_pages(addr, efi_size_in_pages(bs));
+	*buffer = (void *)(uintptr_t)addr;
+	*size = bs;
+error:
+	EFI_CALL(f->close(f));
+	free(info);
 	return ret;
 }
 
@@ -1663,6 +1685,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
 					  efi_uintn_t source_size,
 					  efi_handle_t *image_handle)
 {
+	struct efi_device_path *dp, *fp;
 	struct efi_loaded_image *info = NULL;
 	struct efi_loaded_image_obj **image_obj =
 		(struct efi_loaded_image_obj **)image_handle;
@@ -1682,36 +1705,53 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
 	}
 
 	if (!source_buffer) {
-		struct efi_device_path *dp, *fp;
-
-		ret = efi_load_image_from_path(file_path, &source_buffer);
+		ret = efi_load_image_from_path(file_path, &source_buffer,
+					       &source_size);
 		if (ret != EFI_SUCCESS)
-			goto failure;
+			goto error;
 		/*
 		 * split file_path which contains both the device and
 		 * file parts:
 		 */
 		efi_dp_split_file_path(file_path, &dp, &fp);
-		ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
-		if (ret != EFI_SUCCESS)
-			goto failure;
 	} else {
 		/* In this case, file_path is the "device" path, i.e.
 		 * something like a HARDWARE_DEVICE:MEMORY_MAPPED
 		 */
-		ret = efi_setup_loaded_image(file_path, NULL, image_obj, &info);
+		u64 addr;
+		void *dest_buffer;
+
+		ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
+					 EFI_RUNTIME_SERVICES_CODE,
+					 efi_size_in_pages(source_size), &addr);
 		if (ret != EFI_SUCCESS)
 			goto error;
+		dest_buffer = (void *)(uintptr_t)addr;
+		memcpy(dest_buffer, source_buffer, source_size);
+		source_buffer = dest_buffer;
+
+		dp = file_path;
+		fp = NULL;
 	}
+	ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
+	if (ret != EFI_SUCCESS)
+		goto error_invalid_image;
 	(*image_obj)->entry = efi_load_pe(*image_obj, source_buffer, info);
 	if (!(*image_obj)->entry) {
 		ret = EFI_UNSUPPORTED;
-		goto failure;
+		goto error_invalid_image;
 	}
+	/* Update the type of the allocated memory */
+	efi_add_memory_map((uintptr_t)source_buffer,
+			   efi_size_in_pages(source_size),
+			   info->image_code_type, false);
 	info->system_table = &systab;
 	info->parent_handle = parent_image;
 	return EFI_EXIT(EFI_SUCCESS);
-failure:
+error_invalid_image:
+	/* The image is invalid. Release all associated resources. */
+	efi_free_pages((uintptr_t)source_buffer,
+		       efi_size_in_pages(source_size));
 	efi_delete_handle(*image_handle);
 	*image_handle = NULL;
 	free(info);
-- 
2.20.1

[U-Boot] [PATCH v2 2/4] efi_loader: set entry point in efi_load_pe()

[Heinrich Schuchardt]

Up to now efi_load_pe() returns the entry point or NULL in case of an
error. This does not allow to return correct error codes from LoadImage().

Let efi_load_pe() return a status code and fill in the entry point in the
corresponding field of the image object.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	correct a comment
---
 cmd/bootefi.c                     | 13 ++++-------
 include/efi_loader.h              |  4 ++--
 lib/efi_loader/efi_boottime.c     |  6 ++---
 lib/efi_loader/efi_image_loader.c | 37 ++++++++++++++++++-------------
 4 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index ee685d8644..7f9913c0ee 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -266,9 +266,6 @@ static efi_status_t do_bootefi_exec(void *efi,
 	struct efi_loaded_image_obj *image_obj = NULL;
 	struct efi_loaded_image *loaded_image_info = NULL;
 
-	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
-				     struct efi_system_table *st);
-
 	/*
 	 * Special case for efi payload not loaded from disk, such as
 	 * 'bootefi hello' or for example payload loaded directly into
@@ -300,11 +297,9 @@ static efi_status_t do_bootefi_exec(void *efi,
 		goto err_prepare;
 
 	/* Load the EFI payload */
-	entry = efi_load_pe(image_obj, efi, loaded_image_info);
-	if (!entry) {
-		ret = EFI_LOAD_ERROR;
+	ret = efi_load_pe(image_obj, efi, loaded_image_info);
+	if (ret != EFI_SUCCESS)
 		goto err_prepare;
-	}
 
 	if (memdp) {
 		struct efi_device_path_memory *mdp = (void *)memdp;
@@ -319,14 +314,14 @@ static efi_status_t do_bootefi_exec(void *efi,
 		"{ro,boot}(blob)0000000000000000");
 
 	/* Call our payload! */
-	debug("%s: Jumping to 0x%p\n", __func__, entry);
+	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
 
 	if (setjmp(&image_obj->exit_jmp)) {
 		ret = image_obj->exit_status;
 		goto err_prepare;
 	}
 
-	ret = efi_do_enter(&image_obj->header, &systab, entry);
+	ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry);
 
 err_prepare:
 	/* image has returned, loaded-image obj goes *poof*: */
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 6a98981ebb..3ce43f7a6f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -301,8 +301,8 @@ efi_status_t efi_set_watchdog(unsigned long timeout);
 /* Called from places to check whether a timer expired */
 void efi_timer_check(void);
 /* PE loader implementation */
-void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
-		  struct efi_loaded_image *loaded_image_info);
+efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
+			 struct efi_loaded_image *loaded_image_info);
 /* Called once to store the pristine gd pointer */
 void efi_save_gd(void);
 /* Special case handler for error/abort that just tries to dtrt to get
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 7abdcf6da6..7a61a905f4 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1736,11 +1736,9 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
 	ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
 	if (ret != EFI_SUCCESS)
 		goto error_invalid_image;
-	(*image_obj)->entry = efi_load_pe(*image_obj, source_buffer, info);
-	if (!(*image_obj)->entry) {
-		ret = EFI_UNSUPPORTED;
+	ret = efi_load_pe(*image_obj, source_buffer, info);
+	if (ret != EFI_SUCCESS)
 		goto error_invalid_image;
-	}
 	/* Update the type of the allocated memory */
 	efi_add_memory_map((uintptr_t)source_buffer,
 			   efi_size_in_pages(source_size),
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index a18ce0a570..b55c2840e8 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -193,13 +193,19 @@ static void efi_set_code_and_data_type(
 	}
 }
 
-/*
+/**
+ * efi_load_pe() - relocate EFI binary
+ *
  * This function loads all sections from a PE binary into a newly reserved
- * piece of memory. On successful load it then returns the entry point for
- * the binary. Otherwise NULL.
+ * piece of memory. On success the entry point is returned as handle->entry.
+ *
+ * @handle:		loaded image handle
+ * @efi:		pointer to the EFI binary
+ * @loaded_image_info:	loaded image protocol
+ * Return:		status code
  */
-void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
-		  struct efi_loaded_image *loaded_image_info)
+efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
+			 struct efi_loaded_image *loaded_image_info)
 {
 	IMAGE_NT_HEADERS32 *nt;
 	IMAGE_DOS_HEADER *dos;
@@ -210,7 +216,6 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 	const IMAGE_BASE_RELOCATION *rel;
 	unsigned long rel_size;
 	int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC;
-	void *entry;
 	uint64_t image_base;
 	uint64_t image_size;
 	unsigned long virt_size = 0;
@@ -219,13 +224,13 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 	dos = efi;
 	if (dos->e_magic != IMAGE_DOS_SIGNATURE) {
 		printf("%s: Invalid DOS Signature\n", __func__);
-		return NULL;
+		return EFI_LOAD_ERROR;
 	}
 
 	nt = (void *) ((char *)efi + dos->e_lfanew);
 	if (nt->Signature != IMAGE_NT_SIGNATURE) {
 		printf("%s: Invalid NT Signature\n", __func__);
-		return NULL;
+		return EFI_LOAD_ERROR;
 	}
 
 	for (i = 0; machines[i]; i++)
@@ -237,7 +242,7 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 	if (!supported) {
 		printf("%s: Machine type 0x%04x is not supported\n",
 		       __func__, nt->FileHeader.Machine);
-		return NULL;
+		return EFI_LOAD_ERROR;
 	}
 
 	/* Calculate upper virtual address boundary */
@@ -263,9 +268,9 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 		if (!efi_reloc) {
 			printf("%s: Could not allocate %lu bytes\n",
 			       __func__, virt_size);
-			return NULL;
+			return EFI_OUT_OF_RESOURCES;
 		}
-		entry = efi_reloc + opt->AddressOfEntryPoint;
+		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
 		rel_size = opt->DataDirectory[rel_idx].Size;
 		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
 		virt_size = ALIGN(virt_size, opt->SectionAlignment);
@@ -279,16 +284,16 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 		if (!efi_reloc) {
 			printf("%s: Could not allocate %lu bytes\n",
 			       __func__, virt_size);
-			return NULL;
+			return EFI_OUT_OF_RESOURCES;
 		}
-		entry = efi_reloc + opt->AddressOfEntryPoint;
+		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
 		rel_size = opt->DataDirectory[rel_idx].Size;
 		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
 		virt_size = ALIGN(virt_size, opt->SectionAlignment);
 	} else {
 		printf("%s: Invalid optional header magic %x\n", __func__,
 		       nt->OptionalHeader.Magic);
-		return NULL;
+		return EFI_LOAD_ERROR;
 	}
 
 	/* Load sections into RAM */
@@ -306,7 +311,7 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 				(unsigned long)image_base) != EFI_SUCCESS) {
 		efi_free_pages((uintptr_t) efi_reloc,
 			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
-		return NULL;
+		return EFI_LOAD_ERROR;
 	}
 
 	/* Flush cache */
@@ -320,5 +325,5 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 	handle->reloc_base = efi_reloc;
 	handle->reloc_size = virt_size;
 
-	return entry;
+	return EFI_SUCCESS;
 }
-- 
2.20.1

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Heinrich Schuchardt]

Remove the duplicate code in efi_do_enter() and use efi_start_image() to
start the image invoked by the bootefi command.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	use EFI_CALL
---
 cmd/bootefi.c                 | 22 +---------------------
 include/efi_loader.h          |  4 ++++
 lib/efi_loader/efi_boottime.c |  6 +++---
 3 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 7f9913c0ee..a2d38256e9 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -133,20 +133,6 @@ done:
 	return ret;
 }
 
-static efi_status_t efi_do_enter(
-			efi_handle_t image_handle, struct efi_system_table *st,
-			EFIAPI efi_status_t (*entry)(
-				efi_handle_t image_handle,
-				struct efi_system_table *st))
-{
-	efi_status_t ret = EFI_LOAD_ERROR;
-
-	if (entry)
-		ret = entry(image_handle, st);
-	st->boottime->exit(image_handle, ret, 0, NULL);
-	return ret;
-}
-
 /*
  * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
  *
@@ -315,13 +301,7 @@ static efi_status_t do_bootefi_exec(void *efi,
 
 	/* Call our payload! */
 	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
-
-	if (setjmp(&image_obj->exit_jmp)) {
-		ret = image_obj->exit_status;
-		goto err_prepare;
-	}
-
-	ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry);
+	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
 
 err_prepare:
 	/* image has returned, loaded-image obj goes *poof*: */
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3ce43f7a6f..512880ab8f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -320,6 +320,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle);
 void efi_delete_handle(efi_handle_t obj);
 /* Call this to validate a handle and find the EFI object for it */
 struct efi_object *efi_search_obj(const efi_handle_t handle);
+/* Start image */
+efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
+				    efi_uintn_t *exit_data_size,
+				    u16 **exit_data);
 /* Find a protocol on a handle */
 efi_status_t efi_search_protocol(const efi_handle_t handle,
 				 const efi_guid_t *protocol_guid,
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 7a61a905f4..6c4e2f82ba 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1770,9 +1770,9 @@ error:
  *
  * Return: status code
  */
-static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
-					   efi_uintn_t *exit_data_size,
-					   u16 **exit_data)
+efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
+				    efi_uintn_t *exit_data_size,
+				    u16 **exit_data)
 {
 	struct efi_loaded_image_obj *image_obj =
 		(struct efi_loaded_image_obj *)image_handle;
-- 
2.20.1

[U-Boot] [PATCH v2 4/4] efi_loader: clean up bootefi_test_prepare()

[Heinrich Schuchardt]

Free resources upon failure.

Correct the function description.

As there is no need for any special address in the dummy memory device
path passed via the EFI_LOADED_IMAGE_PROTOCOL simply use 0 as address.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	new patch
---
 cmd/bootefi.c | 58 +++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index a2d38256e9..9d9ccdd31a 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -318,38 +318,46 @@ err_add_protocol:
 /**
  * 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.
+ * Prepare to run a test as if it were provided by a loaded image.
  *
- * @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
- * @load_options_path: U-Boot environment variable to use as load options
- * @return 0 if OK, -ve on error
+ * @image_objp:		pointer to be set to the loaded image handle
+ * @loaded_image_infop:	pointer to be set to the loaded image protocol
+ * @path:		dummy file path used to construct the device path
+ *			set in the loaded image protocol
+ * @load_options_path:	name of a U-Boot environment variable. Its value is
+ *			set as load options in the loaded image protocol.
+ * Return:		status code
  */
 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, const char *load_options_path)
+		 struct efi_loaded_image **loaded_image_infop, const char *path,
+		 const char *load_options_path)
 {
+	efi_status_t ret;
+
 	/* 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_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
 	if (!bootefi_device_path)
 		return EFI_OUT_OF_RESOURCES;
+
 	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
-	if (!bootefi_image_path)
-		return EFI_OUT_OF_RESOURCES;
+	if (!bootefi_image_path) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto failure;
+	}
 
-	return bootefi_run_prepare(load_options_path, bootefi_device_path,
-				   bootefi_image_path, image_objp,
-				   loaded_image_infop);
+	ret = bootefi_run_prepare(load_options_path, bootefi_device_path,
+				  bootefi_image_path, image_objp,
+				  loaded_image_infop);
+	if (ret == EFI_SUCCESS)
+		return ret;
+
+	efi_free_pool(bootefi_image_path);
+	bootefi_image_path = NULL;
+failure:
+	efi_free_pool(bootefi_device_path);
+	bootefi_device_path = NULL;
+	return ret;
 }
 
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
@@ -431,9 +439,9 @@ 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;
 
-		if (bootefi_test_prepare(&image_obj, &loaded_image_info,
-					 "\\selftest", (uintptr_t)&efi_selftest,
-					 "efi_selftest"))
+		r = bootefi_test_prepare(&image_obj, &loaded_image_info,
+					 "\\selftest", "efi_selftest");
+		if (r != EFI_SUCCESS)
 			return CMD_RET_FAILURE;
 
 		/* Execute the test */
-- 
2.20.1

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Jonathan Gray]

On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
> Remove the duplicate code in efi_do_enter() and use efi_start_image() to
> start the image invoked by the bootefi command.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2
> 	use EFI_CALL

This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with U-Boot
releases after 2019.01.  2019.04 works if this commit is reverted.  With
2019.07 there are conflicts trying to revert it and it is still broken
as released.

f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
commit f69d63fae281ba98c3d063097cf4e95d17f3754d
Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
Date:   Wed Dec 26 13:28:09 2018 +0100

    efi_loader: use efi_start_image() for bootefi
    
    Remove the duplicate code in efi_do_enter() and use efi_start_image() to
    start the image invoked by the bootefi command.
    
    Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

 cmd/bootefi.c                 | 22 +---------------------
 include/efi_loader.h          |  4 ++++
 lib/efi_loader/efi_boottime.c |  6 +++---
 3 files changed, 8 insertions(+), 24 deletions(-)

> ---
>  cmd/bootefi.c                 | 22 +---------------------
>  include/efi_loader.h          |  4 ++++
>  lib/efi_loader/efi_boottime.c |  6 +++---
>  3 files changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 7f9913c0ee..a2d38256e9 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -133,20 +133,6 @@ done:
>  	return ret;
>  }
>  
> -static efi_status_t efi_do_enter(
> -			efi_handle_t image_handle, struct efi_system_table *st,
> -			EFIAPI efi_status_t (*entry)(
> -				efi_handle_t image_handle,
> -				struct efi_system_table *st))
> -{
> -	efi_status_t ret = EFI_LOAD_ERROR;
> -
> -	if (entry)
> -		ret = entry(image_handle, st);
> -	st->boottime->exit(image_handle, ret, 0, NULL);
> -	return ret;
> -}
> -
>  /*
>   * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
>   *
> @@ -315,13 +301,7 @@ static efi_status_t do_bootefi_exec(void *efi,
>  
>  	/* Call our payload! */
>  	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> -
> -	if (setjmp(&image_obj->exit_jmp)) {
> -		ret = image_obj->exit_status;
> -		goto err_prepare;
> -	}
> -
> -	ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry);
> +	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
>  
>  err_prepare:
>  	/* image has returned, loaded-image obj goes *poof*: */
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 3ce43f7a6f..512880ab8f 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -320,6 +320,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle);
>  void efi_delete_handle(efi_handle_t obj);
>  /* Call this to validate a handle and find the EFI object for it */
>  struct efi_object *efi_search_obj(const efi_handle_t handle);
> +/* Start image */
> +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> +				    efi_uintn_t *exit_data_size,
> +				    u16 **exit_data);
>  /* Find a protocol on a handle */
>  efi_status_t efi_search_protocol(const efi_handle_t handle,
>  				 const efi_guid_t *protocol_guid,
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 7a61a905f4..6c4e2f82ba 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1770,9 +1770,9 @@ error:
>   *
>   * Return: status code
>   */
> -static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> -					   efi_uintn_t *exit_data_size,
> -					   u16 **exit_data)
> +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> +				    efi_uintn_t *exit_data_size,
> +				    u16 **exit_data)
>  {
>  	struct efi_loaded_image_obj *image_obj =
>  		(struct efi_loaded_image_obj *)image_handle;
> -- 
> 2.20.1
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Mark Kettenis]

> Date: Thu, 18 Jul 2019 16:00:16 +1000
> From: Jonathan Gray <jsg@jsg.id.au>
> 
> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
> > Remove the duplicate code in efi_do_enter() and use efi_start_image() to
> > start the image invoked by the bootefi command.
> > 
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> > v2
> > 	use EFI_CALL
> 
> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with U-Boot
> releases after 2019.01.  2019.04 works if this commit is reverted.  With
> 2019.07 there are conflicts trying to revert it and it is still broken
> as released.
> 
> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Date:   Wed Dec 26 13:28:09 2018 +0100
> 
>     efi_loader: use efi_start_image() for bootefi
>     
>     Remove the duplicate code in efi_do_enter() and use efi_start_image() to
>     start the image invoked by the bootefi command.
>     
>     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
>  cmd/bootefi.c                 | 22 +---------------------
>  include/efi_loader.h          |  4 ++++
>  lib/efi_loader/efi_boottime.c |  6 +++---
>  3 files changed, 8 insertions(+), 24 deletions(-)

Hi Jonathan,

With this commit the OpenBSD bootloader (an EFI application) still
boots, but the loaded OpenBSD kernel doesn't isn't it?

I bet the problem here is that efi_start_image() sets
efi_is_direct_boot to false, which means that efi_exit_caches() which
runs as a result of calling ExitBootServices() no longer
flushes/disables the caches on 32-bit ARM.

We have been here before.  It really isn't possible to flush/disable
the L2 cache on most 32-bit ARM implementations without SoC-specific
code.  And having such code in the early kernel bootstrap code isn't
really possible.

The comments in the code suggest that loading an EFI Linux kernel
directly from U-Boot somehow works without flushing the caches.  But I
don't understand how.  But I'm pretty sure this change break booting
non-EFI Linux kernels through grub on 32-bit ARM platforms as well.

The UEFI spec has the following text (UEFI 2.5 section 2.3.5 "AArch32
Platforms"):

  Implementations of boot services will enable architecturally
  manageable caches and TLBs i.e., those that can be managed directly
  using CP15 operations using mechanisms and procedures defined in the
  ARM Architecture Reference Manual. They should not enable caches
  requiring platform information to manage or invoke non-architectural
  cache/TLB lockdown mechanisms

This suggests that the real problem here is that U-Boot enables the L2
cache on i.MX6 which violates the spec.  But things will probably run
notably slower without the L2 cache enabled.  Maybe the answer is to
just flush/disable the L2 cache when ExitBootServices() is called?


> > ---
> >  cmd/bootefi.c                 | 22 +---------------------
> >  include/efi_loader.h          |  4 ++++
> >  lib/efi_loader/efi_boottime.c |  6 +++---
> >  3 files changed, 8 insertions(+), 24 deletions(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 7f9913c0ee..a2d38256e9 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -133,20 +133,6 @@ done:
> >  	return ret;
> >  }
> >  
> > -static efi_status_t efi_do_enter(
> > -			efi_handle_t image_handle, struct efi_system_table *st,
> > -			EFIAPI efi_status_t (*entry)(
> > -				efi_handle_t image_handle,
> > -				struct efi_system_table *st))
> > -{
> > -	efi_status_t ret = EFI_LOAD_ERROR;
> > -
> > -	if (entry)
> > -		ret = entry(image_handle, st);
> > -	st->boottime->exit(image_handle, ret, 0, NULL);
> > -	return ret;
> > -}
> > -
> >  /*
> >   * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
> >   *
> > @@ -315,13 +301,7 @@ static efi_status_t do_bootefi_exec(void *efi,
> >  
> >  	/* Call our payload! */
> >  	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> > -
> > -	if (setjmp(&image_obj->exit_jmp)) {
> > -		ret = image_obj->exit_status;
> > -		goto err_prepare;
> > -	}
> > -
> > -	ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry);
> > +	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> >  
> >  err_prepare:
> >  	/* image has returned, loaded-image obj goes *poof*: */
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 3ce43f7a6f..512880ab8f 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -320,6 +320,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle);
> >  void efi_delete_handle(efi_handle_t obj);
> >  /* Call this to validate a handle and find the EFI object for it */
> >  struct efi_object *efi_search_obj(const efi_handle_t handle);
> > +/* Start image */
> > +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > +				    efi_uintn_t *exit_data_size,
> > +				    u16 **exit_data);
> >  /* Find a protocol on a handle */
> >  efi_status_t efi_search_protocol(const efi_handle_t handle,
> >  				 const efi_guid_t *protocol_guid,
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 7a61a905f4..6c4e2f82ba 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1770,9 +1770,9 @@ error:
> >   *
> >   * Return: status code
> >   */
> > -static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > -					   efi_uintn_t *exit_data_size,
> > -					   u16 **exit_data)
> > +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > +				    efi_uintn_t *exit_data_size,
> > +				    u16 **exit_data)
> >  {
> >  	struct efi_loaded_image_obj *image_obj =
> >  		(struct efi_loaded_image_obj *)image_handle;
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Jonathan Gray]

On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
> > Date: Thu, 18 Jul 2019 16:00:16 +1000
> > From: Jonathan Gray <jsg@jsg.id.au>
> > 
> > On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
> > > Remove the duplicate code in efi_do_enter() and use efi_start_image() to
> > > start the image invoked by the bootefi command.
> > > 
> > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > > v2
> > > 	use EFI_CALL
> > 
> > This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with U-Boot
> > releases after 2019.01.  2019.04 works if this commit is reverted.  With
> > 2019.07 there are conflicts trying to revert it and it is still broken
> > as released.
> > 
> > f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
> > commit f69d63fae281ba98c3d063097cf4e95d17f3754d
> > Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Date:   Wed Dec 26 13:28:09 2018 +0100
> > 
> >     efi_loader: use efi_start_image() for bootefi
> >     
> >     Remove the duplicate code in efi_do_enter() and use efi_start_image() to
> >     start the image invoked by the bootefi command.
> >     
> >     Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > 
> >  cmd/bootefi.c                 | 22 +---------------------
> >  include/efi_loader.h          |  4 ++++
> >  lib/efi_loader/efi_boottime.c |  6 +++---
> >  3 files changed, 8 insertions(+), 24 deletions(-)
> 
> Hi Jonathan,
> 
> With this commit the OpenBSD bootloader (an EFI application) still
> boots, but the loaded OpenBSD kernel doesn't isn't it?

Yes, when it fails the last thing on serial is:

## Starting EFI application at 12000000 ...
>> OpenBSD/armv7 BOOTARM 1.3
boot> 
booting sd0a:/bsd: 4572484+689312+238360+561608 [298268+120+314400+278666]=0x0

> 
> I bet the problem here is that efi_start_image() sets
> efi_is_direct_boot to false, which means that efi_exit_caches() which
> runs as a result of calling ExitBootServices() no longer
> flushes/disables the caches on 32-bit ARM.

Indeed, removing 'efi_is_direct_boot = false;' from
efi_start_image() allows me to boot multiuser on cubox with 2019.07.

> 
> We have been here before.  It really isn't possible to flush/disable
> the L2 cache on most 32-bit ARM implementations without SoC-specific
> code.  And having such code in the early kernel bootstrap code isn't
> really possible.
> 
> The comments in the code suggest that loading an EFI Linux kernel
> directly from U-Boot somehow works without flushing the caches.  But I
> don't understand how.  But I'm pretty sure this change break booting
> non-EFI Linux kernels through grub on 32-bit ARM platforms as well.
> 
> The UEFI spec has the following text (UEFI 2.5 section 2.3.5 "AArch32
> Platforms"):
> 
>   Implementations of boot services will enable architecturally
>   manageable caches and TLBs i.e., those that can be managed directly
>   using CP15 operations using mechanisms and procedures defined in the
>   ARM Architecture Reference Manual. They should not enable caches
>   requiring platform information to manage or invoke non-architectural
>   cache/TLB lockdown mechanisms
> 
> This suggests that the real problem here is that U-Boot enables the L2
> cache on i.MX6 which violates the spec.  But things will probably run
> notably slower without the L2 cache enabled.  Maybe the answer is to
> just flush/disable the L2 cache when ExitBootServices() is called?
> 
> 
> > > ---
> > >  cmd/bootefi.c                 | 22 +---------------------
> > >  include/efi_loader.h          |  4 ++++
> > >  lib/efi_loader/efi_boottime.c |  6 +++---
> > >  3 files changed, 8 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > index 7f9913c0ee..a2d38256e9 100644
> > > --- a/cmd/bootefi.c
> > > +++ b/cmd/bootefi.c
> > > @@ -133,20 +133,6 @@ done:
> > >  	return ret;
> > >  }
> > >  
> > > -static efi_status_t efi_do_enter(
> > > -			efi_handle_t image_handle, struct efi_system_table *st,
> > > -			EFIAPI efi_status_t (*entry)(
> > > -				efi_handle_t image_handle,
> > > -				struct efi_system_table *st))
> > > -{
> > > -	efi_status_t ret = EFI_LOAD_ERROR;
> > > -
> > > -	if (entry)
> > > -		ret = entry(image_handle, st);
> > > -	st->boottime->exit(image_handle, ret, 0, NULL);
> > > -	return ret;
> > > -}
> > > -
> > >  /*
> > >   * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
> > >   *
> > > @@ -315,13 +301,7 @@ static efi_status_t do_bootefi_exec(void *efi,
> > >  
> > >  	/* Call our payload! */
> > >  	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> > > -
> > > -	if (setjmp(&image_obj->exit_jmp)) {
> > > -		ret = image_obj->exit_status;
> > > -		goto err_prepare;
> > > -	}
> > > -
> > > -	ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry);
> > > +	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> > >  
> > >  err_prepare:
> > >  	/* image has returned, loaded-image obj goes *poof*: */
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index 3ce43f7a6f..512880ab8f 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -320,6 +320,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle);
> > >  void efi_delete_handle(efi_handle_t obj);
> > >  /* Call this to validate a handle and find the EFI object for it */
> > >  struct efi_object *efi_search_obj(const efi_handle_t handle);
> > > +/* Start image */
> > > +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > > +				    efi_uintn_t *exit_data_size,
> > > +				    u16 **exit_data);
> > >  /* Find a protocol on a handle */
> > >  efi_status_t efi_search_protocol(const efi_handle_t handle,
> > >  				 const efi_guid_t *protocol_guid,
> > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > > index 7a61a905f4..6c4e2f82ba 100644
> > > --- a/lib/efi_loader/efi_boottime.c
> > > +++ b/lib/efi_loader/efi_boottime.c
> > > @@ -1770,9 +1770,9 @@ error:
> > >   *
> > >   * Return: status code
> > >   */
> > > -static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > > -					   efi_uintn_t *exit_data_size,
> > > -					   u16 **exit_data)
> > > +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > > +				    efi_uintn_t *exit_data_size,
> > > +				    u16 **exit_data)
> > >  {
> > >  	struct efi_loaded_image_obj *image_obj =
> > >  		(struct efi_loaded_image_obj *)image_handle;
> > > -- 
> > > 2.20.1
> > > 
> > > _______________________________________________
> > > U-Boot mailing list
> > > U-Boot at lists.denx.de
> > > https://lists.denx.de/listinfo/u-boot
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Heinrich Schuchardt]

On 7/18/19 11:16 AM, Jonathan Gray wrote:
> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
>>> Date: Thu, 18 Jul 2019 16:00:16 +1000
>>> From: Jonathan Gray <jsg@jsg.id.au>
>>>
>>> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
>>>> Remove the duplicate code in efi_do_enter() and use efi_start_image() to
>>>> start the image invoked by the bootefi command.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>> v2
>>>> 	use EFI_CALL
>>>
>>> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with U-Boot
>>> releases after 2019.01.  2019.04 works if this commit is reverted.  With
>>> 2019.07 there are conflicts trying to revert it and it is still broken
>>> as released.
>>>
>>> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
>>> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
>>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Date:   Wed Dec 26 13:28:09 2018 +0100
>>>
>>>      efi_loader: use efi_start_image() for bootefi
>>>
>>>      Remove the duplicate code in efi_do_enter() and use efi_start_image() to
>>>      start the image invoked by the bootefi command.
>>>
>>>      Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>
>>>   cmd/bootefi.c                 | 22 +---------------------
>>>   include/efi_loader.h          |  4 ++++
>>>   lib/efi_loader/efi_boottime.c |  6 +++---
>>>   3 files changed, 8 insertions(+), 24 deletions(-)
>>
>> Hi Jonathan,
>>
>> With this commit the OpenBSD bootloader (an EFI application) still
>> boots, but the loaded OpenBSD kernel doesn't isn't it?
>
> Yes, when it fails the last thing on serial is:
>
> ## Starting EFI application at 12000000 ...
>>> OpenBSD/armv7 BOOTARM 1.3
> boot>
> booting sd0a:/bsd: 4572484+689312+238360+561608 [298268+120+314400+278666]=0x0
>
>>
>> I bet the problem here is that efi_start_image() sets
>> efi_is_direct_boot to false, which means that efi_exit_caches() which
>> runs as a result of calling ExitBootServices() no longer
>> flushes/disables the caches on 32-bit ARM.
>
> Indeed, removing 'efi_is_direct_boot = false;' from
> efi_start_image() allows me to boot multiuser on cubox with 2019.07.
>
>>
>> We have been here before.  It really isn't possible to flush/disable
>> the L2 cache on most 32-bit ARM implementations without SoC-specific
>> code.  And having such code in the early kernel bootstrap code isn't
>> really possible.
>>
>> The comments in the code suggest that loading an EFI Linux kernel
>> directly from U-Boot somehow works without flushing the caches.  But I
>> don't understand how.  But I'm pretty sure this change break booting
>> non-EFI Linux kernels through grub on 32-bit ARM platforms as well.
>>
>> The UEFI spec has the following text (UEFI 2.5 section 2.3.5 "AArch32
>> Platforms"):
>>
>>    Implementations of boot services will enable architecturally
>>    manageable caches and TLBs i.e., those that can be managed directly
>>    using CP15 operations using mechanisms and procedures defined in the
>>    ARM Architecture Reference Manual. They should not enable caches
>>    requiring platform information to manage or invoke non-architectural
>>    cache/TLB lockdown mechanisms
>>
>> This suggests that the real problem here is that U-Boot enables the L2
>> cache on i.MX6 which violates the spec

Thanks for your investigation.

To which spec are you relating?

UEFI 2.8 spec, chapter 2.3.5 AArch32 Platforms:

The core will be configured as follows (common across all processor
architecture revisions): Instruction and Data caches enabled

GRUB is invalidating the cache before starting Linux by calling
grub_arch_sync_caches ((void *) linux_addr, linux_size);.

I think the BSD bootloader will also have to clear caches after loading
the main program.

@Alex:
When should we call clean_up_before_linux()?
In the first StartImage() call?
Always in ExitBootServices()?

Best regards

Heinrich

>> But things will probably run
>> notably slower without the L2 cache enabled.  Maybe the answer is to
>> just flush/disable the L2 cache when ExitBootServices() is called?
>>
>>
>>>> ---
>>>>   cmd/bootefi.c                 | 22 +---------------------
>>>>   include/efi_loader.h          |  4 ++++
>>>>   lib/efi_loader/efi_boottime.c |  6 +++---
>>>>   3 files changed, 8 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>> index 7f9913c0ee..a2d38256e9 100644
>>>> --- a/cmd/bootefi.c
>>>> +++ b/cmd/bootefi.c
>>>> @@ -133,20 +133,6 @@ done:
>>>>   	return ret;
>>>>   }
>>>>
>>>> -static efi_status_t efi_do_enter(
>>>> -			efi_handle_t image_handle, struct efi_system_table *st,
>>>> -			EFIAPI efi_status_t (*entry)(
>>>> -				efi_handle_t image_handle,
>>>> -				struct efi_system_table *st))
>>>> -{
>>>> -	efi_status_t ret = EFI_LOAD_ERROR;
>>>> -
>>>> -	if (entry)
>>>> -		ret = entry(image_handle, st);
>>>> -	st->boottime->exit(image_handle, ret, 0, NULL);
>>>> -	return ret;
>>>> -}
>>>> -
>>>>   /*
>>>>    * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
>>>>    *
>>>> @@ -315,13 +301,7 @@ static efi_status_t do_bootefi_exec(void *efi,
>>>>
>>>>   	/* Call our payload! */
>>>>   	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
>>>> -
>>>> -	if (setjmp(&image_obj->exit_jmp)) {
>>>> -		ret = image_obj->exit_status;
>>>> -		goto err_prepare;
>>>> -	}
>>>> -
>>>> -	ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry);
>>>> +	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
>>>>
>>>>   err_prepare:
>>>>   	/* image has returned, loaded-image obj goes *poof*: */
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 3ce43f7a6f..512880ab8f 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -320,6 +320,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle);
>>>>   void efi_delete_handle(efi_handle_t obj);
>>>>   /* Call this to validate a handle and find the EFI object for it */
>>>>   struct efi_object *efi_search_obj(const efi_handle_t handle);
>>>> +/* Start image */
>>>> +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>>> +				    efi_uintn_t *exit_data_size,
>>>> +				    u16 **exit_data);
>>>>   /* Find a protocol on a handle */
>>>>   efi_status_t efi_search_protocol(const efi_handle_t handle,
>>>>   				 const efi_guid_t *protocol_guid,
>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>> index 7a61a905f4..6c4e2f82ba 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -1770,9 +1770,9 @@ error:
>>>>    *
>>>>    * Return: status code
>>>>    */
>>>> -static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>>> -					   efi_uintn_t *exit_data_size,
>>>> -					   u16 **exit_data)
>>>> +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>>> +				    efi_uintn_t *exit_data_size,
>>>> +				    u16 **exit_data)
>>>>   {
>>>>   	struct efi_loaded_image_obj *image_obj =
>>>>   		(struct efi_loaded_image_obj *)image_handle;
>>>> --
>>>> 2.20.1
>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> https://lists.denx.de/listinfo/u-boot
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> https://lists.denx.de/listinfo/u-boot
>

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Alexander Graf]

On 18.07.19 19:33, Heinrich Schuchardt wrote:
> On 7/18/19 11:16 AM, Jonathan Gray wrote:
>> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
>>>> Date: Thu, 18 Jul 2019 16:00:16 +1000
>>>> From: Jonathan Gray <jsg@jsg.id.au>
>>>>
>>>> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
>>>>> Remove the duplicate code in efi_do_enter() and use 
>>>>> efi_start_image() to
>>>>> start the image invoked by the bootefi command.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>> v2
>>>>>     use EFI_CALL
>>>>
>>>> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with 
>>>> U-Boot
>>>> releases after 2019.01.  2019.04 works if this commit is reverted.  
>>>> With
>>>> 2019.07 there are conflicts trying to revert it and it is still broken
>>>> as released.
>>>>
>>>> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
>>>> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
>>>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> Date:   Wed Dec 26 13:28:09 2018 +0100
>>>>
>>>>      efi_loader: use efi_start_image() for bootefi
>>>>
>>>>      Remove the duplicate code in efi_do_enter() and use 
>>>> efi_start_image() to
>>>>      start the image invoked by the bootefi command.
>>>>
>>>>      Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>
>>>>   cmd/bootefi.c                 | 22 +---------------------
>>>>   include/efi_loader.h          |  4 ++++
>>>>   lib/efi_loader/efi_boottime.c |  6 +++---
>>>>   3 files changed, 8 insertions(+), 24 deletions(-)
>>>
>>> Hi Jonathan,
>>>
>>> With this commit the OpenBSD bootloader (an EFI application) still
>>> boots, but the loaded OpenBSD kernel doesn't isn't it?
>>
>> Yes, when it fails the last thing on serial is:
>>
>> ## Starting EFI application at 12000000 ...
>>>> OpenBSD/armv7 BOOTARM 1.3
>> boot>
>> booting sd0a:/bsd: 4572484+689312+238360+561608 
>> [298268+120+314400+278666]=0x0
>>
>>>
>>> I bet the problem here is that efi_start_image() sets
>>> efi_is_direct_boot to false, which means that efi_exit_caches() which
>>> runs as a result of calling ExitBootServices() no longer
>>> flushes/disables the caches on 32-bit ARM.
>>
>> Indeed, removing 'efi_is_direct_boot = false;' from
>> efi_start_image() allows me to boot multiuser on cubox with 2019.07.
>>
>>>
>>> We have been here before.  It really isn't possible to flush/disable
>>> the L2 cache on most 32-bit ARM implementations without SoC-specific
>>> code.  And having such code in the early kernel bootstrap code isn't
>>> really possible.
>>>
>>> The comments in the code suggest that loading an EFI Linux kernel
>>> directly from U-Boot somehow works without flushing the caches.  But I
>>> don't understand how.  But I'm pretty sure this change break booting
>>> non-EFI Linux kernels through grub on 32-bit ARM platforms as well.
>>>
>>> The UEFI spec has the following text (UEFI 2.5 section 2.3.5 "AArch32
>>> Platforms"):
>>>
>>>    Implementations of boot services will enable architecturally
>>>    manageable caches and TLBs i.e., those that can be managed directly
>>>    using CP15 operations using mechanisms and procedures defined in the
>>>    ARM Architecture Reference Manual. They should not enable caches
>>>    requiring platform information to manage or invoke non-architectural
>>>    cache/TLB lockdown mechanisms
>>>
>>> This suggests that the real problem here is that U-Boot enables the L2
>>> cache on i.MX6 which violates the spec
>
> Thanks for your investigation.
>
> To which spec are you relating?
>
> UEFI 2.8 spec, chapter 2.3.5 AArch32 Platforms:
>
> The core will be configured as follows (common across all processor
> architecture revisions): Instruction and Data caches enabled
>
> GRUB is invalidating the cache before starting Linux by calling
> grub_arch_sync_caches ((void *) linux_addr, linux_size);.
>
> I think the BSD bootloader will also have to clear caches after loading
> the main program.
>
> @Alex:
> When should we call clean_up_before_linux()?
> In the first StartImage() call?
> Always in ExitBootServices()?


The problem with 32bit ARM grub was that it simply never called 
StartImage(), because it did not execute UEFI kernels. Instead it tried 
to simulate the legacy boot path.

Or in other words: If there ever was a StartImage() call, we do not need 
to disable caches, as that means the kernel is called indirectly. Has 
that logic changed?


Alex

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Heinrich Schuchardt]

On 7/18/19 7:40 PM, Alexander Graf wrote:
>
> On 18.07.19 19:33, Heinrich Schuchardt wrote:
>> On 7/18/19 11:16 AM, Jonathan Gray wrote:
>>> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
>>>>> Date: Thu, 18 Jul 2019 16:00:16 +1000
>>>>> From: Jonathan Gray <jsg@jsg.id.au>
>>>>>
>>>>> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
>>>>>> Remove the duplicate code in efi_do_enter() and use
>>>>>> efi_start_image() to
>>>>>> start the image invoked by the bootefi command.
>>>>>>
>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>> ---
>>>>>> v2
>>>>>>     use EFI_CALL
>>>>>
>>>>> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with
>>>>> U-Boot
>>>>> releases after 2019.01.  2019.04 works if this commit is reverted.
>>>>> With
>>>>> 2019.07 there are conflicts trying to revert it and it is still broken
>>>>> as released.
>>>>>
>>>>> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
>>>>> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
>>>>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> Date:   Wed Dec 26 13:28:09 2018 +0100
>>>>>
>>>>>      efi_loader: use efi_start_image() for bootefi
>>>>>
>>>>>      Remove the duplicate code in efi_do_enter() and use
>>>>> efi_start_image() to
>>>>>      start the image invoked by the bootefi command.
>>>>>
>>>>>      Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>
>>>>>   cmd/bootefi.c                 | 22 +---------------------
>>>>>   include/efi_loader.h          |  4 ++++
>>>>>   lib/efi_loader/efi_boottime.c |  6 +++---
>>>>>   3 files changed, 8 insertions(+), 24 deletions(-)
>>>>
>>>> Hi Jonathan,
>>>>
>>>> With this commit the OpenBSD bootloader (an EFI application) still
>>>> boots, but the loaded OpenBSD kernel doesn't isn't it?
>>>
>>> Yes, when it fails the last thing on serial is:
>>>
>>> ## Starting EFI application at 12000000 ...
>>>>> OpenBSD/armv7 BOOTARM 1.3
>>> boot>
>>> booting sd0a:/bsd: 4572484+689312+238360+561608
>>> [298268+120+314400+278666]=0x0
>>>
>>>>
>>>> I bet the problem here is that efi_start_image() sets
>>>> efi_is_direct_boot to false, which means that efi_exit_caches() which
>>>> runs as a result of calling ExitBootServices() no longer
>>>> flushes/disables the caches on 32-bit ARM.
>>>
>>> Indeed, removing 'efi_is_direct_boot = false;' from
>>> efi_start_image() allows me to boot multiuser on cubox with 2019.07.
>>>
>>>>
>>>> We have been here before.  It really isn't possible to flush/disable
>>>> the L2 cache on most 32-bit ARM implementations without SoC-specific
>>>> code.  And having such code in the early kernel bootstrap code isn't
>>>> really possible.
>>>>
>>>> The comments in the code suggest that loading an EFI Linux kernel
>>>> directly from U-Boot somehow works without flushing the caches.  But I
>>>> don't understand how.  But I'm pretty sure this change break booting
>>>> non-EFI Linux kernels through grub on 32-bit ARM platforms as well.
>>>>
>>>> The UEFI spec has the following text (UEFI 2.5 section 2.3.5 "AArch32
>>>> Platforms"):
>>>>
>>>>    Implementations of boot services will enable architecturally
>>>>    manageable caches and TLBs i.e., those that can be managed directly
>>>>    using CP15 operations using mechanisms and procedures defined in the
>>>>    ARM Architecture Reference Manual. They should not enable caches
>>>>    requiring platform information to manage or invoke non-architectural
>>>>    cache/TLB lockdown mechanisms
>>>>
>>>> This suggests that the real problem here is that U-Boot enables the L2
>>>> cache on i.MX6 which violates the spec
>>
>> Thanks for your investigation.
>>
>> To which spec are you relating?
>>
>> UEFI 2.8 spec, chapter 2.3.5 AArch32 Platforms:
>>
>> The core will be configured as follows (common across all processor
>> architecture revisions): Instruction and Data caches enabled
>>
>> GRUB is invalidating the cache before starting Linux by calling
>> grub_arch_sync_caches ((void *) linux_addr, linux_size);.
>>
>> I think the BSD bootloader will also have to clear caches after loading
>> the main program.
>>
>> @Alex:
>> When should we call clean_up_before_linux()?
>> In the first StartImage() call?
>> Always in ExitBootServices()?
>
>
> The problem with 32bit ARM grub was that it simply never called
> StartImage(), because it did not execute UEFI kernels. Instead it tried
> to simulate the legacy boot path.
>
> Or in other words: If there ever was a StartImage() call, we do not need
> to disable caches, as that means the kernel is called indirectly. Has
> that logic changed?

We always call StartImage() where we set efi_is_direct_boot = false.
Hence we never call cleanup_before_linux().

Regards

Heinrich

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Alexander Graf]

On 18.07.19 19:33, Heinrich Schuchardt wrote:
> On 7/18/19 11:16 AM, Jonathan Gray wrote:
>> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
>>>> Date: Thu, 18 Jul 2019 16:00:16 +1000
>>>> From: Jonathan Gray <jsg@jsg.id.au>
>>>>
>>>> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
>>>>> Remove the duplicate code in efi_do_enter() and use 
>>>>> efi_start_image() to
>>>>> start the image invoked by the bootefi command.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>> v2
>>>>>     use EFI_CALL
>>>>
>>>> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with 
>>>> U-Boot
>>>> releases after 2019.01.  2019.04 works if this commit is reverted.  
>>>> With
>>>> 2019.07 there are conflicts trying to revert it and it is still broken
>>>> as released.
>>>>
>>>> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
>>>> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
>>>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> Date:   Wed Dec 26 13:28:09 2018 +0100
>>>>
>>>>      efi_loader: use efi_start_image() for bootefi
>>>>
>>>>      Remove the duplicate code in efi_do_enter() and use 
>>>> efi_start_image() to
>>>>      start the image invoked by the bootefi command.
>>>>
>>>>      Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>
>>>>   cmd/bootefi.c                 | 22 +---------------------
>>>>   include/efi_loader.h          |  4 ++++
>>>>   lib/efi_loader/efi_boottime.c |  6 +++---
>>>>   3 files changed, 8 insertions(+), 24 deletions(-)
>>>
>>> Hi Jonathan,
>>>
>>> With this commit the OpenBSD bootloader (an EFI application) still
>>> boots, but the loaded OpenBSD kernel doesn't isn't it?
>>
>> Yes, when it fails the last thing on serial is:
>>
>> ## Starting EFI application at 12000000 ...
>>>> OpenBSD/armv7 BOOTARM 1.3
>> boot>
>> booting sd0a:/bsd: 4572484+689312+238360+561608 
>> [298268+120+314400+278666]=0x0
>>
>>>
>>> I bet the problem here is that efi_start_image() sets
>>> efi_is_direct_boot to false, which means that efi_exit_caches() which
>>> runs as a result of calling ExitBootServices() no longer
>>> flushes/disables the caches on 32-bit ARM.
>>
>> Indeed, removing 'efi_is_direct_boot = false;' from
>> efi_start_image() allows me to boot multiuser on cubox with 2019.07.


Heinrich asked me to clarify again in the mail thread.

The UEFI spec says that caches on 32bit ARM are supposed to be *enabled* 
always. That means throughout boot time, as well as after 
ExitBootServices(). So the behavior with efi_is_direct_boot = false is 
the expected behavior according to the spec.

The reason we have the hack above in the tree is that grub had a nasty 
deficiency on 32bit ARM where it called into the Linux kernel using the 
legacy boot protocol which again expected caches to be disabled.

The problem you're facing here sounds to me like a bug in the OpenBSD 
loader that resembles the bug in grub. Instead of calling into a BSD 
kernel code path that expects caches to already be enabled, it expects 
to come up with caches disabled.

You *could* solve this generically by adding functionality akin to 
clean_up_before_linux() inside the BSD loader. The downside of that is 
that the code would be platform specific, as L2 caches used to be 
enabled/disabled via MMIOs and not via architected registers in the old 
days.


Alex

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Mark Kettenis]

> From: Alexander Graf <agraf@csgraf.de>
> Date: Thu, 18 Jul 2019 20:50:39 +0200
> 
> On 18.07.19 19:33, Heinrich Schuchardt wrote:
> > On 7/18/19 11:16 AM, Jonathan Gray wrote:
> >> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
> >>>> Date: Thu, 18 Jul 2019 16:00:16 +1000
> >>>> From: Jonathan Gray <jsg@jsg.id.au>
> >>>>
> >>>> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
> >>>>> Remove the duplicate code in efi_do_enter() and use 
> >>>>> efi_start_image() to
> >>>>> start the image invoked by the bootefi command.
> >>>>>
> >>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>> ---
> >>>>> v2
> >>>>>     use EFI_CALL
> >>>>
> >>>> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with 
> >>>> U-Boot
> >>>> releases after 2019.01.  2019.04 works if this commit is reverted.  
> >>>> With
> >>>> 2019.07 there are conflicts trying to revert it and it is still broken
> >>>> as released.
> >>>>
> >>>> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
> >>>> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
> >>>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>> Date:   Wed Dec 26 13:28:09 2018 +0100
> >>>>
> >>>>      efi_loader: use efi_start_image() for bootefi
> >>>>
> >>>>      Remove the duplicate code in efi_do_enter() and use 
> >>>> efi_start_image() to
> >>>>      start the image invoked by the bootefi command.
> >>>>
> >>>>      Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>
> >>>>   cmd/bootefi.c                 | 22 +---------------------
> >>>>   include/efi_loader.h          |  4 ++++
> >>>>   lib/efi_loader/efi_boottime.c |  6 +++---
> >>>>   3 files changed, 8 insertions(+), 24 deletions(-)
> >>>
> >>> Hi Jonathan,
> >>>
> >>> With this commit the OpenBSD bootloader (an EFI application) still
> >>> boots, but the loaded OpenBSD kernel doesn't isn't it?
> >>
> >> Yes, when it fails the last thing on serial is:
> >>
> >> ## Starting EFI application at 12000000 ...
> >>>> OpenBSD/armv7 BOOTARM 1.3
> >> boot>
> >> booting sd0a:/bsd: 4572484+689312+238360+561608 
> >> [298268+120+314400+278666]=0x0
> >>
> >>>
> >>> I bet the problem here is that efi_start_image() sets
> >>> efi_is_direct_boot to false, which means that efi_exit_caches() which
> >>> runs as a result of calling ExitBootServices() no longer
> >>> flushes/disables the caches on 32-bit ARM.
> >>
> >> Indeed, removing 'efi_is_direct_boot = false;' from
> >> efi_start_image() allows me to boot multiuser on cubox with 2019.07.
> 
> 
> Heinrich asked me to clarify again in the mail thread.
> 
> The UEFI spec says that caches on 32bit ARM are supposed to be *enabled* 
> always. That means throughout boot time, as well as after 
> ExitBootServices(). So the behavior with efi_is_direct_boot = false is 
> the expected behavior according to the spec.
> 
> The reason we have the hack above in the tree is that grub had a nasty 
> deficiency on 32bit ARM where it called into the Linux kernel using the 
> legacy boot protocol which again expected caches to be disabled.
> 
> The problem you're facing here sounds to me like a bug in the OpenBSD 
> loader that resembles the bug in grub. Instead of calling into a BSD 
> kernel code path that expects caches to already be enabled, it expects 
> to come up with caches disabled.

I don't think we assume that the kernel is called with the
architecturally defined caches disabled.  As far as I know, OpenBSD
works fine with a recent U-Boot on Cortex-A7 systems that don't have
an non-architectural L2 cache.

> You *could* solve this generically by adding functionality akin to 
> clean_up_before_linux() inside the BSD loader. The downside of that is 
> that the code would be platform specific, as L2 caches used to be 
> enabled/disabled via MMIOs and not via architected registers in the old 
> days.

Messing with the non-architectural L2 cache is someting we defenitely
don't want to do in the OpenBSD bootloader and kernel bootstrap code.
We have a single bootloader/kernel that works on all supported armv7
systems!

I quoted UEFI 2.5 upthread, but 2.8 retains the same text in section
2.3.5:

  Implementations of boot services will enable architecturally
  manageable caches and TLBs i.e., those that can be managed directly
  using CP15 operations using mechanisms and procedures defined in the
  ARM Architecture Reference Manual. They should not enable caches
  requiring platform information to manage or invoke non-architectural
  cache/TLB lockdown mechanisms

So I'd say the problem here is that U-Boot passes control to the EFI
payload with the L2 cache enabled.

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Heinrich Schuchardt]

On 7/19/19 1:07 AM, Mark Kettenis wrote:
>> From: Alexander Graf <agraf@csgraf.de>
>> Date: Thu, 18 Jul 2019 20:50:39 +0200
>>
>> On 18.07.19 19:33, Heinrich Schuchardt wrote:
>>> On 7/18/19 11:16 AM, Jonathan Gray wrote:
>>>> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
>>>>>> Date: Thu, 18 Jul 2019 16:00:16 +1000
>>>>>> From: Jonathan Gray <jsg@jsg.id.au>
>>>>>>
>>>>>> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
>>>>>>> Remove the duplicate code in efi_do_enter() and use
>>>>>>> efi_start_image() to
>>>>>>> start the image invoked by the bootefi command.
>>>>>>>
>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>> ---
>>>>>>> v2
>>>>>>>      use EFI_CALL
>>>>>>
>>>>>> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with
>>>>>> U-Boot
>>>>>> releases after 2019.01.  2019.04 works if this commit is reverted.
>>>>>> With
>>>>>> 2019.07 there are conflicts trying to revert it and it is still broken
>>>>>> as released.
>>>>>>
>>>>>> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
>>>>>> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
>>>>>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>> Date:   Wed Dec 26 13:28:09 2018 +0100
>>>>>>
>>>>>>       efi_loader: use efi_start_image() for bootefi
>>>>>>
>>>>>>       Remove the duplicate code in efi_do_enter() and use
>>>>>> efi_start_image() to
>>>>>>       start the image invoked by the bootefi command.
>>>>>>
>>>>>>       Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>
>>>>>>    cmd/bootefi.c                 | 22 +---------------------
>>>>>>    include/efi_loader.h          |  4 ++++
>>>>>>    lib/efi_loader/efi_boottime.c |  6 +++---
>>>>>>    3 files changed, 8 insertions(+), 24 deletions(-)
>>>>>
>>>>> Hi Jonathan,
>>>>>
>>>>> With this commit the OpenBSD bootloader (an EFI application) still
>>>>> boots, but the loaded OpenBSD kernel doesn't isn't it?
>>>>
>>>> Yes, when it fails the last thing on serial is:
>>>>
>>>> ## Starting EFI application at 12000000 ...
>>>>>> OpenBSD/armv7 BOOTARM 1.3
>>>> boot>
>>>> booting sd0a:/bsd: 4572484+689312+238360+561608
>>>> [298268+120+314400+278666]=0x0
>>>>
>>>>>
>>>>> I bet the problem here is that efi_start_image() sets
>>>>> efi_is_direct_boot to false, which means that efi_exit_caches() which
>>>>> runs as a result of calling ExitBootServices() no longer
>>>>> flushes/disables the caches on 32-bit ARM.
>>>>
>>>> Indeed, removing 'efi_is_direct_boot = false;' from
>>>> efi_start_image() allows me to boot multiuser on cubox with 2019.07.
>>
>>
>> Heinrich asked me to clarify again in the mail thread.
>>
>> The UEFI spec says that caches on 32bit ARM are supposed to be *enabled*
>> always. That means throughout boot time, as well as after
>> ExitBootServices(). So the behavior with efi_is_direct_boot = false is
>> the expected behavior according to the spec.
>>
>> The reason we have the hack above in the tree is that grub had a nasty
>> deficiency on 32bit ARM where it called into the Linux kernel using the
>> legacy boot protocol which again expected caches to be disabled.
>>
>> The problem you're facing here sounds to me like a bug in the OpenBSD
>> loader that resembles the bug in grub. Instead of calling into a BSD
>> kernel code path that expects caches to already be enabled, it expects
>> to come up with caches disabled.
>
> I don't think we assume that the kernel is called with the
> architecturally defined caches disabled.  As far as I know, OpenBSD
> works fine with a recent U-Boot on Cortex-A7 systems that don't have
> an non-architectural L2 cache.
>
>> You *could* solve this generically by adding functionality akin to
>> clean_up_before_linux() inside the BSD loader. The downside of that is
>> that the code would be platform specific, as L2 caches used to be
>> enabled/disabled via MMIOs and not via architected registers in the old
>> days.
>
> Messing with the non-architectural L2 cache is someting we defenitely
> don't want to do in the OpenBSD bootloader and kernel bootstrap code.
> We have a single bootloader/kernel that works on all supported armv7
> systems!
>
> I quoted UEFI 2.5 upthread, but 2.8 retains the same text in section
> 2.3.5:
>
>    Implementations of boot services will enable architecturally
>    manageable caches and TLBs i.e., those that can be managed directly
>    using CP15 operations using mechanisms and procedures defined in the
>    ARM Architecture Reference Manual. They should not enable caches
>    requiring platform information to manage or invoke non-architectural
>    cache/TLB lockdown mechanisms
>
> So I'd say the problem here is that U-Boot passes control to the EFI
> payload with the L2 cache enabled.
>

Hello Mark,

your last observation concerned an i.MX6 board. Does this processor have
any cache that falls under the cited paragraph? A link to the processor
documentation would be helpful.

Best regards

Heinrich

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Jonathan Gray]

On Fri, Jul 19, 2019 at 07:43:17AM +0200, Heinrich Schuchardt wrote:
> On 7/19/19 1:07 AM, Mark Kettenis wrote:
> > > From: Alexander Graf <agraf@csgraf.de>
> > > Date: Thu, 18 Jul 2019 20:50:39 +0200
> > > 
> > > On 18.07.19 19:33, Heinrich Schuchardt wrote:
> > > > On 7/18/19 11:16 AM, Jonathan Gray wrote:
> > > > > On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
> > > > > > > Date: Thu, 18 Jul 2019 16:00:16 +1000
> > > > > > > From: Jonathan Gray <jsg@jsg.id.au>
> > > > > > > 
> > > > > > > On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
> > > > > > > > Remove the duplicate code in efi_do_enter() and use
> > > > > > > > efi_start_image() to
> > > > > > > > start the image invoked by the bootefi command.
> > > > > > > > 
> > > > > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > > > ---
> > > > > > > > v2
> > > > > > > >  ????????use EFI_CALL
> > > > > > > 
> > > > > > > This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with
> > > > > > > U-Boot
> > > > > > > releases after 2019.01.?? 2019.04 works if this commit is reverted.
> > > > > > > With
> > > > > > > 2019.07 there are conflicts trying to revert it and it is still broken
> > > > > > > as released.
> > > > > > > 
> > > > > > > f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
> > > > > > > commit f69d63fae281ba98c3d063097cf4e95d17f3754d
> > > > > > > Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > > Date:???? Wed Dec 26 13:28:09 2018 +0100
> > > > > > > 
> > > > > > >  ???????? efi_loader: use efi_start_image() for bootefi
> > > > > > > 
> > > > > > >  ???????? Remove the duplicate code in efi_do_enter() and use
> > > > > > > efi_start_image() to
> > > > > > >  ???????? start the image invoked by the bootefi command.
> > > > > > > 
> > > > > > >  ???????? Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > > 
> > > > > > >  ?? cmd/bootefi.c???????????????????????????????? | 22 +---------------------
> > > > > > >  ?? include/efi_loader.h?????????????????? |?? 4 ++++
> > > > > > >  ?? lib/efi_loader/efi_boottime.c |?? 6 +++---
> > > > > > >  ?? 3 files changed, 8 insertions(+), 24 deletions(-)
> > > > > > 
> > > > > > Hi Jonathan,
> > > > > > 
> > > > > > With this commit the OpenBSD bootloader (an EFI application) still
> > > > > > boots, but the loaded OpenBSD kernel doesn't isn't it?
> > > > > 
> > > > > Yes, when it fails the last thing on serial is:
> > > > > 
> > > > > ## Starting EFI application at 12000000 ...
> > > > > > > OpenBSD/armv7 BOOTARM 1.3
> > > > > boot>
> > > > > booting sd0a:/bsd: 4572484+689312+238360+561608
> > > > > [298268+120+314400+278666]=0x0
> > > > > 
> > > > > > 
> > > > > > I bet the problem here is that efi_start_image() sets
> > > > > > efi_is_direct_boot to false, which means that efi_exit_caches() which
> > > > > > runs as a result of calling ExitBootServices() no longer
> > > > > > flushes/disables the caches on 32-bit ARM.
> > > > > 
> > > > > Indeed, removing 'efi_is_direct_boot = false;' from
> > > > > efi_start_image() allows me to boot multiuser on cubox with 2019.07.
> > > 
> > > 
> > > Heinrich asked me to clarify again in the mail thread.
> > > 
> > > The UEFI spec says that caches on 32bit ARM are supposed to be *enabled*
> > > always. That means throughout boot time, as well as after
> > > ExitBootServices(). So the behavior with efi_is_direct_boot = false is
> > > the expected behavior according to the spec.
> > > 
> > > The reason we have the hack above in the tree is that grub had a nasty
> > > deficiency on 32bit ARM where it called into the Linux kernel using the
> > > legacy boot protocol which again expected caches to be disabled.
> > > 
> > > The problem you're facing here sounds to me like a bug in the OpenBSD
> > > loader that resembles the bug in grub. Instead of calling into a BSD
> > > kernel code path that expects caches to already be enabled, it expects
> > > to come up with caches disabled.
> > 
> > I don't think we assume that the kernel is called with the
> > architecturally defined caches disabled.  As far as I know, OpenBSD
> > works fine with a recent U-Boot on Cortex-A7 systems that don't have
> > an non-architectural L2 cache.
> > 
> > > You *could* solve this generically by adding functionality akin to
> > > clean_up_before_linux() inside the BSD loader. The downside of that is
> > > that the code would be platform specific, as L2 caches used to be
> > > enabled/disabled via MMIOs and not via architected registers in the old
> > > days.
> > 
> > Messing with the non-architectural L2 cache is someting we defenitely
> > don't want to do in the OpenBSD bootloader and kernel bootstrap code.
> > We have a single bootloader/kernel that works on all supported armv7
> > systems!
> > 
> > I quoted UEFI 2.5 upthread, but 2.8 retains the same text in section
> > 2.3.5:
> > 
> >    Implementations of boot services will enable architecturally
> >    manageable caches and TLBs i.e., those that can be managed directly
> >    using CP15 operations using mechanisms and procedures defined in the
> >    ARM Architecture Reference Manual. They should not enable caches
> >    requiring platform information to manage or invoke non-architectural
> >    cache/TLB lockdown mechanisms
> > 
> > So I'd say the problem here is that U-Boot passes control to the EFI
> > payload with the L2 cache enabled.
> > 
> 
> Hello Mark,
> 
> your last observation concerned an i.MX6 board. Does this processor have
> any cache that falls under the cited paragraph? A link to the processor
> documentation would be helpful.
> 
> Best regards
> 
> Heinrich

i.MX6 has a PL310 L2 cache which has it's own TRM from Arm.
NXP now requires an account to download IMX6DQRM.pdf sadly.

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0246c/DDI0246C_l2cc_pl310_r2p0_trm.pdf
"The cache controller is configured using memory-mapped registers,
rather than using CP15 instructions"

u-boot/include/configs/mx6_common.h:#define CONFIG_SYS_L2_PL310

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Heinrich Schuchardt]

On 7/19/19 8:14 AM, Jonathan Gray wrote:
> On Fri, Jul 19, 2019 at 07:43:17AM +0200, Heinrich Schuchardt wrote:
>> On 7/19/19 1:07 AM, Mark Kettenis wrote:
>>>> From: Alexander Graf <agraf@csgraf.de>
>>>> Date: Thu, 18 Jul 2019 20:50:39 +0200
>>>>
>>>> On 18.07.19 19:33, Heinrich Schuchardt wrote:
>>>>> On 7/18/19 11:16 AM, Jonathan Gray wrote:
>>>>>> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
>>>>>>>> Date: Thu, 18 Jul 2019 16:00:16 +1000
>>>>>>>> From: Jonathan Gray <jsg@jsg.id.au>
>>>>>>>>
>>>>>>>> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt wrote:
>>>>>>>>> Remove the duplicate code in efi_do_enter() and use
>>>>>>>>> efi_start_image() to
>>>>>>>>> start the image invoked by the bootefi command.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>> ---
>>>>>>>>> v2
>>>>>>>>>   ????????use EFI_CALL
>>>>>>>>
>>>>>>>> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with
>>>>>>>> U-Boot
>>>>>>>> releases after 2019.01.?? 2019.04 works if this commit is reverted.
>>>>>>>> With
>>>>>>>> 2019.07 there are conflicts trying to revert it and it is still broken
>>>>>>>> as released.
>>>>>>>>
>>>>>>>> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
>>>>>>>> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
>>>>>>>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>> Date:???? Wed Dec 26 13:28:09 2018 +0100
>>>>>>>>
>>>>>>>>   ???????? efi_loader: use efi_start_image() for bootefi
>>>>>>>>
>>>>>>>>   ???????? Remove the duplicate code in efi_do_enter() and use
>>>>>>>> efi_start_image() to
>>>>>>>>   ???????? start the image invoked by the bootefi command.
>>>>>>>>
>>>>>>>>   ???????? Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>
>>>>>>>>   ?? cmd/bootefi.c???????????????????????????????? | 22 +---------------------
>>>>>>>>   ?? include/efi_loader.h?????????????????? |?? 4 ++++
>>>>>>>>   ?? lib/efi_loader/efi_boottime.c |?? 6 +++---
>>>>>>>>   ?? 3 files changed, 8 insertions(+), 24 deletions(-)
>>>>>>>
>>>>>>> Hi Jonathan,
>>>>>>>
>>>>>>> With this commit the OpenBSD bootloader (an EFI application) still
>>>>>>> boots, but the loaded OpenBSD kernel doesn't isn't it?
>>>>>>
>>>>>> Yes, when it fails the last thing on serial is:
>>>>>>
>>>>>> ## Starting EFI application at 12000000 ...
>>>>>>>> OpenBSD/armv7 BOOTARM 1.3
>>>>>> boot>
>>>>>> booting sd0a:/bsd: 4572484+689312+238360+561608
>>>>>> [298268+120+314400+278666]=0x0
>>>>>>
>>>>>>>
>>>>>>> I bet the problem here is that efi_start_image() sets
>>>>>>> efi_is_direct_boot to false, which means that efi_exit_caches() which
>>>>>>> runs as a result of calling ExitBootServices() no longer
>>>>>>> flushes/disables the caches on 32-bit ARM.
>>>>>>
>>>>>> Indeed, removing 'efi_is_direct_boot = false;' from
>>>>>> efi_start_image() allows me to boot multiuser on cubox with 2019.07.
>>>>
>>>>
>>>> Heinrich asked me to clarify again in the mail thread.
>>>>
>>>> The UEFI spec says that caches on 32bit ARM are supposed to be *enabled*
>>>> always. That means throughout boot time, as well as after
>>>> ExitBootServices(). So the behavior with efi_is_direct_boot = false is
>>>> the expected behavior according to the spec.
>>>>
>>>> The reason we have the hack above in the tree is that grub had a nasty
>>>> deficiency on 32bit ARM where it called into the Linux kernel using the
>>>> legacy boot protocol which again expected caches to be disabled.
>>>>
>>>> The problem you're facing here sounds to me like a bug in the OpenBSD
>>>> loader that resembles the bug in grub. Instead of calling into a BSD
>>>> kernel code path that expects caches to already be enabled, it expects
>>>> to come up with caches disabled.
>>>
>>> I don't think we assume that the kernel is called with the
>>> architecturally defined caches disabled.  As far as I know, OpenBSD
>>> works fine with a recent U-Boot on Cortex-A7 systems that don't have
>>> an non-architectural L2 cache.
>>>
>>>> You *could* solve this generically by adding functionality akin to
>>>> clean_up_before_linux() inside the BSD loader. The downside of that is
>>>> that the code would be platform specific, as L2 caches used to be
>>>> enabled/disabled via MMIOs and not via architected registers in the old
>>>> days.
>>>
>>> Messing with the non-architectural L2 cache is someting we defenitely
>>> don't want to do in the OpenBSD bootloader and kernel bootstrap code.
>>> We have a single bootloader/kernel that works on all supported armv7
>>> systems!
>>>
>>> I quoted UEFI 2.5 upthread, but 2.8 retains the same text in section
>>> 2.3.5:
>>>
>>>     Implementations of boot services will enable architecturally
>>>     manageable caches and TLBs i.e., those that can be managed directly
>>>     using CP15 operations using mechanisms and procedures defined in the
>>>     ARM Architecture Reference Manual. They should not enable caches
>>>     requiring platform information to manage or invoke non-architectural
>>>     cache/TLB lockdown mechanisms
>>>
>>> So I'd say the problem here is that U-Boot passes control to the EFI
>>> payload with the L2 cache enabled.
>>>
>>
>> Hello Mark,
>>
>> your last observation concerned an i.MX6 board. Does this processor have
>> any cache that falls under the cited paragraph? A link to the processor
>> documentation would be helpful.
>>
>> Best regards
>>
>> Heinrich
>
> i.MX6 has a PL310 L2 cache which has it's own TRM from Arm.
> NXP now requires an account to download IMX6DQRM.pdf sadly.
>
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0246c/DDI0246C_l2cc_pl310_r2p0_trm.pdf
> "The cache controller is configured using memory-mapped registers,
> rather than using CP15 instructions"
>
> u-boot/include/configs/mx6_common.h:#define CONFIG_SYS_L2_PL310
>

mx6cuboxi_defconfig has CONFIG_SYS_L2CACHE_OFF=N.

Doesn't this mean that we conform to the UEFI specification?

Best regards

Heinrich

[U-Boot] [PATCH v2 3/4] efi_loader: use efi_start_image() for bootefi

[Heinrich Schuchardt]

On 7/19/19 7:52 PM, Heinrich Schuchardt wrote:
> On 7/19/19 8:14 AM, Jonathan Gray wrote:
>> On Fri, Jul 19, 2019 at 07:43:17AM +0200, Heinrich Schuchardt wrote:
>>> On 7/19/19 1:07 AM, Mark Kettenis wrote:
>>>>> From: Alexander Graf <agraf@csgraf.de>
>>>>> Date: Thu, 18 Jul 2019 20:50:39 +0200
>>>>>
>>>>> On 18.07.19 19:33, Heinrich Schuchardt wrote:
>>>>>> On 7/18/19 11:16 AM, Jonathan Gray wrote:
>>>>>>> On Thu, Jul 18, 2019 at 10:39:57AM +0200, Mark Kettenis wrote:
>>>>>>>>> Date: Thu, 18 Jul 2019 16:00:16 +1000
>>>>>>>>> From: Jonathan Gray <jsg@jsg.id.au>
>>>>>>>>>
>>>>>>>>> On Fri, Feb 08, 2019 at 07:46:49PM +0100, Heinrich Schuchardt
>>>>>>>>> wrote:
>>>>>>>>>> Remove the duplicate code in efi_do_enter() and use
>>>>>>>>>> efi_start_image() to
>>>>>>>>>> start the image invoked by the bootefi command.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>> ---
>>>>>>>>>> v2
>>>>>>>>>>   ????????use EFI_CALL
>>>>>>>>>
>>>>>>>>> This commit broke booting OpenBSD/armv7 kernels on mx6cuboxi with
>>>>>>>>> U-Boot
>>>>>>>>> releases after 2019.01.?? 2019.04 works if this commit is
>>>>>>>>> reverted.
>>>>>>>>> With
>>>>>>>>> 2019.07 there are conflicts trying to revert it and it is still
>>>>>>>>> broken
>>>>>>>>> as released.
>>>>>>>>>
>>>>>>>>> f69d63fae281ba98c3d063097cf4e95d17f3754d is the first bad commit
>>>>>>>>> commit f69d63fae281ba98c3d063097cf4e95d17f3754d
>>>>>>>>> Author: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>> Date:???? Wed Dec 26 13:28:09 2018 +0100
>>>>>>>>>
>>>>>>>>>   ???????? efi_loader: use efi_start_image() for bootefi
>>>>>>>>>
>>>>>>>>>   ???????? Remove the duplicate code in efi_do_enter() and use
>>>>>>>>> efi_start_image() to
>>>>>>>>>   ???????? start the image invoked by the bootefi command.
>>>>>>>>>
>>>>>>>>>   ???????? Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>>>
>>>>>>>>>   ?? cmd/bootefi.c???????????????????????????????? | 22
>>>>>>>>> +---------------------
>>>>>>>>>   ?? include/efi_loader.h?????????????????? |?? 4 ++++
>>>>>>>>>   ?? lib/efi_loader/efi_boottime.c |?? 6 +++---
>>>>>>>>>   ?? 3 files changed, 8 insertions(+), 24 deletions(-)
>>>>>>>>
>>>>>>>> Hi Jonathan,
>>>>>>>>
>>>>>>>> With this commit the OpenBSD bootloader (an EFI application) still
>>>>>>>> boots, but the loaded OpenBSD kernel doesn't isn't it?
>>>>>>>
>>>>>>> Yes, when it fails the last thing on serial is:
>>>>>>>
>>>>>>> ## Starting EFI application at 12000000 ...
>>>>>>>>> OpenBSD/armv7 BOOTARM 1.3
>>>>>>> boot>
>>>>>>> booting sd0a:/bsd: 4572484+689312+238360+561608
>>>>>>> [298268+120+314400+278666]=0x0
>>>>>>>
>>>>>>>>
>>>>>>>> I bet the problem here is that efi_start_image() sets
>>>>>>>> efi_is_direct_boot to false, which means that efi_exit_caches()
>>>>>>>> which
>>>>>>>> runs as a result of calling ExitBootServices() no longer
>>>>>>>> flushes/disables the caches on 32-bit ARM.
>>>>>>>
>>>>>>> Indeed, removing 'efi_is_direct_boot = false;' from
>>>>>>> efi_start_image() allows me to boot multiuser on cubox with 2019.07.
>>>>>
>>>>>
>>>>> Heinrich asked me to clarify again in the mail thread.
>>>>>
>>>>> The UEFI spec says that caches on 32bit ARM are supposed to be
>>>>> *enabled*
>>>>> always. That means throughout boot time, as well as after
>>>>> ExitBootServices(). So the behavior with efi_is_direct_boot = false is
>>>>> the expected behavior according to the spec.
>>>>>
>>>>> The reason we have the hack above in the tree is that grub had a nasty
>>>>> deficiency on 32bit ARM where it called into the Linux kernel using
>>>>> the
>>>>> legacy boot protocol which again expected caches to be disabled.
>>>>>
>>>>> The problem you're facing here sounds to me like a bug in the OpenBSD
>>>>> loader that resembles the bug in grub. Instead of calling into a BSD
>>>>> kernel code path that expects caches to already be enabled, it expects
>>>>> to come up with caches disabled.
>>>>
>>>> I don't think we assume that the kernel is called with the
>>>> architecturally defined caches disabled.  As far as I know, OpenBSD
>>>> works fine with a recent U-Boot on Cortex-A7 systems that don't have
>>>> an non-architectural L2 cache.
>>>>
>>>>> You *could* solve this generically by adding functionality akin to
>>>>> clean_up_before_linux() inside the BSD loader. The downside of that is
>>>>> that the code would be platform specific, as L2 caches used to be
>>>>> enabled/disabled via MMIOs and not via architected registers in the
>>>>> old
>>>>> days.
>>>>
>>>> Messing with the non-architectural L2 cache is someting we defenitely
>>>> don't want to do in the OpenBSD bootloader and kernel bootstrap code.
>>>> We have a single bootloader/kernel that works on all supported armv7
>>>> systems!
>>>>
>>>> I quoted UEFI 2.5 upthread, but 2.8 retains the same text in section
>>>> 2.3.5:
>>>>
>>>>     Implementations of boot services will enable architecturally
>>>>     manageable caches and TLBs i.e., those that can be managed directly
>>>>     using CP15 operations using mechanisms and procedures defined in
>>>> the
>>>>     ARM Architecture Reference Manual. They should not enable caches
>>>>     requiring platform information to manage or invoke
>>>> non-architectural
>>>>     cache/TLB lockdown mechanisms
>>>>
>>>> So I'd say the problem here is that U-Boot passes control to the EFI
>>>> payload with the L2 cache enabled.
>>>>
>>>
>>> Hello Mark,
>>>
>>> your last observation concerned an i.MX6 board. Does this processor have
>>> any cache that falls under the cited paragraph? A link to the processor
>>> documentation would be helpful.
>>>
>>> Best regards
>>>
>>> Heinrich
>>
>> i.MX6 has a PL310 L2 cache which has it's own TRM from Arm.
>> NXP now requires an account to download IMX6DQRM.pdf sadly.
>>
>> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0246c/DDI0246C_l2cc_pl310_r2p0_trm.pdf
>>
>> "The cache controller is configured using memory-mapped registers,
>> rather than using CP15 instructions"
>>
>> u-boot/include/configs/mx6_common.h:#define CONFIG_SYS_L2_PL310
>>
>
> mx6cuboxi_defconfig has CONFIG_SYS_L2CACHE_OFF=N.
>
> Doesn't this mean that we conform to the UEFI specification?

Jonathan can you, please, test booting OpenBSD with
CONFIG_SYS_L2CACHE_OFF=Y.

Best regards

Heinrich