[U-Boot] [PATCH 0/4] efi_loader implent unloading of images

[U-Boot] [PATCH 0/4] efi_loader implent unloading of images

[Heinrich Schuchardt]

The patch series implements the UnloadImage() boot service and the
unloading of images in Exit().

Heinrich Schuchardt (4):
  efi_loader: mark started images
  efi_loader: move efi_unload_image() down in source
  efi_loader: implement UnloadImage()
  efi_loader: unload applications upon Exit()

 include/efi_api.h                 |   2 +-
 include/efi_loader.h              |  14 ++++
 lib/efi_loader/efi_boottime.c     | 129 +++++++++++++++++++++++-------
 lib/efi_loader/efi_image_loader.c |   2 +
 4 files changed, 118 insertions(+), 29 deletions(-)

--
2.20.1

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Heinrich Schuchardt]

In UnloadImage() we need to know if an image is already started.

Add a field to the handle structure identifying loaded and started images.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          | 13 +++++++++++++
 lib/efi_loader/efi_boottime.c |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3fd9901d66..c2a449e5b6 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -182,6 +182,18 @@ struct efi_handler {
 	struct list_head open_infos;
 };

+/**
+ * enum efi_object_type - type of EFI object
+ *
+ * In UnloadImage we must be able to identify if the handle relates to a
+ * started image.
+ */
+enum efi_object_type {
+	EFI_OBJECT_TYPE_UNDEFINED = 0,
+	EFI_OBJECT_TYPE_LOADED_IMAGE,
+	EFI_OBJECT_TYPE_STARTED_IMAGE,
+};
+
 /**
  * struct efi_object - dereferenced EFI handle
  *
@@ -204,6 +216,7 @@ struct efi_object {
 	struct list_head link;
 	/* The list of protocols */
 	struct list_head protocols;
+	enum efi_object_type type;
 };

 /**
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 3ed08e7c37..dc444fccf6 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
 		free(info);
 		return EFI_OUT_OF_RESOURCES;
 	}
+	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;

 	/* Add internal object to object list */
 	efi_add_handle(&obj->header);
@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	}

 	current_image = image_handle;
+	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
 	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
 	ret = EFI_CALL(image_obj->entry(image_handle, &systab));

--
2.20.1

[U-Boot] [PATCH 2/4] efi_loader: move efi_unload_image() down in source

[Heinrich Schuchardt]

Move efi_unload_image() down in source to avoid forward declaration in
following page.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 46 +++++++++++++++++------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index dc444fccf6..2992af269a 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1744,29 +1744,6 @@ error:
 	return EFI_EXIT(ret);
 }

-/**
- * efi_unload_image() - unload an EFI image
- * @image_handle: handle of the image to be unloaded
- *
- * This function implements the UnloadImage service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * Return: status code
- */
-efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle)
-{
-	struct efi_object *efiobj;
-
-	EFI_ENTRY("%p", image_handle);
-	efiobj = efi_search_obj(image_handle);
-	if (efiobj)
-		list_del(&efiobj->link);
-
-	return EFI_EXIT(EFI_SUCCESS);
-}
-
 /**
  * efi_exit_caches() - fix up caches for EFI payloads if necessary
  */
@@ -2692,6 +2669,29 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	return EFI_CALL(systab.boottime->exit(image_handle, ret, 0, NULL));
 }

+/**
+ * efi_unload_image() - unload an EFI image
+ * @image_handle: handle of the image to be unloaded
+ *
+ * This function implements the UnloadImage service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * Return: status code
+ */
+efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle)
+{
+	struct efi_object *efiobj;
+
+	EFI_ENTRY("%p", image_handle);
+	efiobj = efi_search_obj(image_handle);
+	if (efiobj)
+		list_del(&efiobj->link);
+
+	return EFI_EXIT(EFI_SUCCESS);
+}
+
 /**
  * efi_update_exit_data() - fill exit data parameters of StartImage()
  *
--
2.20.1

[U-Boot] [PATCH 3/4] efi_loader: implement UnloadImage()

[Heinrich Schuchardt]

Implement the UnloadImage() boot service

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_api.h             |  2 +-
 lib/efi_loader/efi_boottime.c | 55 ++++++++++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index 4ebb4a5f59..54c6232079 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -348,7 +348,7 @@ struct efi_loaded_image {
 	aligned_u64 image_size;
 	unsigned int image_code_type;
 	unsigned int image_data_type;
-	unsigned long unload;
+	efi_status_t (EFIAPI *unload)(efi_handle_t image_handle);
 };

 #define EFI_DEVICE_PATH_PROTOCOL_GUID \
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 2992af269a..bbcd66caa6 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2669,6 +2669,20 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	return EFI_CALL(systab.boottime->exit(image_handle, ret, 0, NULL));
 }

+/**
+ * efi_delete_image() - delete loaded image from memory)
+ *
+ * @image_obj:			handle of the loaded image
+ * @loaded_image_protocol:	loaded image protocol
+ */
+static void efi_delete_image(struct efi_loaded_image_obj *image_obj,
+			     struct efi_loaded_image *loaded_image_protocol)
+{
+	efi_free_pages((uintptr_t)loaded_image_protocol->image_base,
+		       efi_size_in_pages(loaded_image_protocol->image_size));
+	efi_delete_handle(&image_obj->header);
+}
+
 /**
  * efi_unload_image() - unload an EFI image
  * @image_handle: handle of the image to be unloaded
@@ -2682,14 +2696,47 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
  */
 efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle)
 {
+	efi_status_t ret = EFI_SUCCESS;
 	struct efi_object *efiobj;
+	struct efi_loaded_image *loaded_image_protocol;

 	EFI_ENTRY("%p", image_handle);
-	efiobj = efi_search_obj(image_handle);
-	if (efiobj)
-		list_del(&efiobj->link);

-	return EFI_EXIT(EFI_SUCCESS);
+	efiobj = efi_search_obj(image_handle);
+	if (!efiobj) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	/* Find the loaded image protocol */
+	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
+					 (void **)&loaded_image_protocol,
+					 NULL, NULL,
+					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
+	if (ret != EFI_SUCCESS) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	switch (efiobj->type) {
+	case EFI_OBJECT_TYPE_STARTED_IMAGE:
+		/* Call the unload function */
+		if (!loaded_image_protocol->unload) {
+			ret = EFI_UNSUPPORTED;
+			goto out;
+		}
+		ret = EFI_CALL(loaded_image_protocol->unload(image_handle));
+		if (ret != EFI_SUCCESS)
+			goto out;
+		break;
+	case EFI_OBJECT_TYPE_LOADED_IMAGE:
+		break;
+	default:
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	efi_delete_image((struct efi_loaded_image_obj *)efiobj,
+			 loaded_image_protocol);
+out:
+	return EFI_EXIT(ret);
 }

 /**
--
2.20.1

[U-Boot] [PATCH 4/4] efi_loader: unload applications upon Exit()

[Heinrich Schuchardt]

Implement unloading of images in the Exit() boot services:

* unload images that are not yet started,
* unload started applications,
* unload drivers returning an error.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h              |  1 +
 lib/efi_loader/efi_boottime.c     | 34 ++++++++++++++++++++++++++-----
 lib/efi_loader/efi_image_loader.c |  2 ++
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index c2a449e5b6..d73c89ac26 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -237,6 +237,7 @@ struct efi_loaded_image_obj {
 	struct jmp_buf_data exit_jmp;
 	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
 				     struct efi_system_table *st);
+	u16 image_type;
 };

 /**
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index bbcd66caa6..51e0bb2fd5 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -13,6 +13,7 @@
 #include <linux/libfdt_env.h>
 #include <u-boot/crc.h>
 #include <bootm.h>
+#include <pe.h>
 #include <watchdog.h>

 DECLARE_GLOBAL_DATA_PTR;
@@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 	 *	 image protocol.
 	 */
 	efi_status_t ret;
-	void *info;
+	struct efi_loaded_image *loaded_image_protocol;
 	struct efi_loaded_image_obj *image_obj =
 		(struct efi_loaded_image_obj *)image_handle;

@@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 		  exit_data_size, exit_data);

 	/* Check parameters */
-	if (image_handle != current_image)
+	if (image_handle != current_image) {
+		ret = EFI_INVALID_PARAMETER;
 		goto out;
+	}
 	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
-					 &info, NULL, NULL,
+					 (void **)&loaded_image_protocol,
+					 NULL, NULL,
 					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
-	if (ret != EFI_SUCCESS)
+	if (ret != EFI_SUCCESS) {
+		ret = EFI_INVALID_PARAMETER;
 		goto out;
+	}
+
+	/* Unloading of unstarted images */
+	switch (image_obj->header.type) {
+	case EFI_OBJECT_TYPE_STARTED_IMAGE:
+		break;
+	case EFI_OBJECT_TYPE_LOADED_IMAGE:
+		efi_delete_image(image_obj, loaded_image_protocol);
+		ret = EFI_SUCCESS;
+		goto out;
+	default:
+		/* Handle does not refer to loaded image */
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;

 	/* Exit data is only foreseen in case of failure. */
 	if (exit_status != EFI_SUCCESS) {
@@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 		if (ret != EFI_SUCCESS)
 			EFI_PRINT("%s: out of memory\n", __func__);
 	}
+	if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
+	    exit_status != EFI_SUCCESS)
+		efi_delete_image(image_obj, loaded_image_protocol);

 	/* Make sure entry/exit counts for EFI world cross-overs match */
 	EFI_EXIT(exit_status);
@@ -2837,7 +2861,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,

 	panic("EFI application exited");
 out:
-	return EFI_EXIT(EFI_INVALID_PARAMETER);
+	return EFI_EXIT(ret);
 }

 /**
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index f8092b6202..13541cfa7a 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -273,6 +273,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
 		image_base = opt->ImageBase;
 		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
+		handle->image_type = opt->Subsystem;
 		efi_reloc = efi_alloc(virt_size,
 				      loaded_image_info->image_code_type);
 		if (!efi_reloc) {
@@ -288,6 +289,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
 		image_base = opt->ImageBase;
 		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
+		handle->image_type = opt->Subsystem;
 		efi_reloc = efi_alloc(virt_size,
 				      loaded_image_info->image_code_type);
 		if (!efi_reloc) {
--
2.20.1

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Takahiro Akashi]

On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
> In UnloadImage() we need to know if an image is already started.
> 
> Add a field to the handle structure identifying loaded and started images.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h          | 13 +++++++++++++
>  lib/efi_loader/efi_boottime.c |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 3fd9901d66..c2a449e5b6 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -182,6 +182,18 @@ struct efi_handler {
>  	struct list_head open_infos;
>  };
> 
> +/**
> + * enum efi_object_type - type of EFI object
> + *
> + * In UnloadImage we must be able to identify if the handle relates to a
> + * started image.
> + */
> +enum efi_object_type {
> +	EFI_OBJECT_TYPE_UNDEFINED = 0,
> +	EFI_OBJECT_TYPE_LOADED_IMAGE,
> +	EFI_OBJECT_TYPE_STARTED_IMAGE,
> +};

It sounds *status*, not *type*.
In a separate patch, you added U_BOOT_FIRMWARE.
We should distinguish status and type for future enhancement and
to avoid any confusion.

Thanks,
-Takahiro Akashi


>  /**
>   * struct efi_object - dereferenced EFI handle
>   *
> @@ -204,6 +216,7 @@ struct efi_object {
>  	struct list_head link;
>  	/* The list of protocols */
>  	struct list_head protocols;
> +	enum efi_object_type type;
>  };
> 
>  /**
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 3ed08e7c37..dc444fccf6 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>  		free(info);
>  		return EFI_OUT_OF_RESOURCES;
>  	}
> +	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
> 
>  	/* Add internal object to object list */
>  	efi_add_handle(&obj->header);
> @@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>  	}
> 
>  	current_image = image_handle;
> +	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
>  	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
>  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> 
> --
> 2.20.1
> 

[U-Boot] [PATCH 4/4] efi_loader: unload applications upon Exit()

[Takahiro Akashi]

On Sat, May 04, 2019 at 10:36:36AM +0200, Heinrich Schuchardt wrote:
> Implement unloading of images in the Exit() boot services:
> 
> * unload images that are not yet started,
> * unload started applications,
> * unload drivers returning an error.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h              |  1 +
>  lib/efi_loader/efi_boottime.c     | 34 ++++++++++++++++++++++++++-----
>  lib/efi_loader/efi_image_loader.c |  2 ++
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index c2a449e5b6..d73c89ac26 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -237,6 +237,7 @@ struct efi_loaded_image_obj {
>  	struct jmp_buf_data exit_jmp;
>  	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
>  				     struct efi_system_table *st);
> +	u16 image_type;
>  };
> 
>  /**
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index bbcd66caa6..51e0bb2fd5 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -13,6 +13,7 @@
>  #include <linux/libfdt_env.h>
>  #include <u-boot/crc.h>
>  #include <bootm.h>
> +#include <pe.h>
>  #include <watchdog.h>
> 
>  DECLARE_GLOBAL_DATA_PTR;
> @@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>  	 *	 image protocol.
>  	 */
>  	efi_status_t ret;
> -	void *info;
> +	struct efi_loaded_image *loaded_image_protocol;
>  	struct efi_loaded_image_obj *image_obj =
>  		(struct efi_loaded_image_obj *)image_handle;
> 
> @@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>  		  exit_data_size, exit_data);
> 
>  	/* Check parameters */
> -	if (image_handle != current_image)
> +	if (image_handle != current_image) {

Does this check make sense even for a driver?

> +		ret = EFI_INVALID_PARAMETER;
>  		goto out;
> +	}
>  	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> -					 &info, NULL, NULL,
> +					 (void **)&loaded_image_protocol,
> +					 NULL, NULL,
>  					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> -	if (ret != EFI_SUCCESS)
> +	if (ret != EFI_SUCCESS) {

Is this check necessary even when 'header.type' is introduced?

> +		ret = EFI_INVALID_PARAMETER;
>  		goto out;
> +	}
> +
> +	/* Unloading of unstarted images */

'Unload' sounds odd when we call efi_delete_image(), doesn't it?

> +	switch (image_obj->header.type) {
> +	case EFI_OBJECT_TYPE_STARTED_IMAGE:
> +		break;
> +	case EFI_OBJECT_TYPE_LOADED_IMAGE:
> +		efi_delete_image(image_obj, loaded_image_protocol);
> +		ret = EFI_SUCCESS;
> +		goto out;
> +	default:
> +		/* Handle does not refer to loaded image */
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
> 
>  	/* Exit data is only foreseen in case of failure. */
>  	if (exit_status != EFI_SUCCESS) {
> @@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>  		if (ret != EFI_SUCCESS)
>  			EFI_PRINT("%s: out of memory\n", __func__);
>  	}
> +	if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
> +	    exit_status != EFI_SUCCESS)
> +		efi_delete_image(image_obj, loaded_image_protocol);

Why not merge those two efi_delete_image()?

-Takahiro Akashi


>  	/* Make sure entry/exit counts for EFI world cross-overs match */
>  	EFI_EXIT(exit_status);
> @@ -2837,7 +2861,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> 
>  	panic("EFI application exited");
>  out:
> -	return EFI_EXIT(EFI_INVALID_PARAMETER);
> +	return EFI_EXIT(ret);
>  }
> 
>  /**
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index f8092b6202..13541cfa7a 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -273,6 +273,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>  		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
>  		image_base = opt->ImageBase;
>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> +		handle->image_type = opt->Subsystem;
>  		efi_reloc = efi_alloc(virt_size,
>  				      loaded_image_info->image_code_type);
>  		if (!efi_reloc) {
> @@ -288,6 +289,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>  		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
>  		image_base = opt->ImageBase;
>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> +		handle->image_type = opt->Subsystem;
>  		efi_reloc = efi_alloc(virt_size,
>  				      loaded_image_info->image_code_type);
>  		if (!efi_reloc) {
> --
> 2.20.1
> 

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Heinrich Schuchardt]

On 5/7/19 5:02 AM, Takahiro Akashi wrote:
> On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
>> In UnloadImage() we need to know if an image is already started.
>>
>> Add a field to the handle structure identifying loaded and started images.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   include/efi_loader.h          | 13 +++++++++++++
>>   lib/efi_loader/efi_boottime.c |  2 ++
>>   2 files changed, 15 insertions(+)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 3fd9901d66..c2a449e5b6 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -182,6 +182,18 @@ struct efi_handler {
>>   	struct list_head open_infos;
>>   };
>>
>> +/**
>> + * enum efi_object_type - type of EFI object
>> + *
>> + * In UnloadImage we must be able to identify if the handle relates to a
>> + * started image.
>> + */
>> +enum efi_object_type {
>> +	EFI_OBJECT_TYPE_UNDEFINED = 0,
>> +	EFI_OBJECT_TYPE_LOADED_IMAGE,
>> +	EFI_OBJECT_TYPE_STARTED_IMAGE,
>> +};
>
> It sounds *status*, not *type*.
> In a separate patch, you added U_BOOT_FIRMWARE.
> We should distinguish status and type for future enhancement and
> to avoid any confusion.

Having both information in the same field makes the evaluation easier.

Currently I see not necessity for more handle types to be
differentiated. Let's change it when the need arises.

Best regards

Heinrich

>
> Thanks,
> -Takahiro Akashi
>
>
>>   /**
>>    * struct efi_object - dereferenced EFI handle
>>    *
>> @@ -204,6 +216,7 @@ struct efi_object {
>>   	struct list_head link;
>>   	/* The list of protocols */
>>   	struct list_head protocols;
>> +	enum efi_object_type type;
>>   };
>>
>>   /**
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 3ed08e7c37..dc444fccf6 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>>   		free(info);
>>   		return EFI_OUT_OF_RESOURCES;
>>   	}
>> +	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
>>
>>   	/* Add internal object to object list */
>>   	efi_add_handle(&obj->header);
>> @@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>   	}
>>
>>   	current_image = image_handle;
>> +	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
>>   	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
>>   	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>
>> --
>> 2.20.1
>>
>

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Takahiro Akashi]

On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
> On 5/7/19 5:02 AM, Takahiro Akashi wrote:
> >On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
> >>In UnloadImage() we need to know if an image is already started.
> >>
> >>Add a field to the handle structure identifying loaded and started images.
> >>
> >>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>---
> >>  include/efi_loader.h          | 13 +++++++++++++
> >>  lib/efi_loader/efi_boottime.c |  2 ++
> >>  2 files changed, 15 insertions(+)
> >>
> >>diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>index 3fd9901d66..c2a449e5b6 100644
> >>--- a/include/efi_loader.h
> >>+++ b/include/efi_loader.h
> >>@@ -182,6 +182,18 @@ struct efi_handler {
> >>  	struct list_head open_infos;
> >>  };
> >>
> >>+/**
> >>+ * enum efi_object_type - type of EFI object
> >>+ *
> >>+ * In UnloadImage we must be able to identify if the handle relates to a
> >>+ * started image.
> >>+ */
> >>+enum efi_object_type {
> >>+	EFI_OBJECT_TYPE_UNDEFINED = 0,
> >>+	EFI_OBJECT_TYPE_LOADED_IMAGE,
> >>+	EFI_OBJECT_TYPE_STARTED_IMAGE,
> >>+};
> >
> >It sounds *status*, not *type*.
> >In a separate patch, you added U_BOOT_FIRMWARE.
> >We should distinguish status and type for future enhancement and
> >to avoid any confusion.
> 
> Having both information in the same field makes the evaluation easier.
> 
> Currently I see not necessity for more handle types to be
> differentiated. Let's change it when the need arises.

I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE
for root node. Then we can rename efi_object_type to efi_image_status.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> >Thanks,
> >-Takahiro Akashi
> >
> >
> >>  /**
> >>   * struct efi_object - dereferenced EFI handle
> >>   *
> >>@@ -204,6 +216,7 @@ struct efi_object {
> >>  	struct list_head link;
> >>  	/* The list of protocols */
> >>  	struct list_head protocols;
> >>+	enum efi_object_type type;
> >>  };
> >>
> >>  /**
> >>diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >>index 3ed08e7c37..dc444fccf6 100644
> >>--- a/lib/efi_loader/efi_boottime.c
> >>+++ b/lib/efi_loader/efi_boottime.c
> >>@@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >>  		free(info);
> >>  		return EFI_OUT_OF_RESOURCES;
> >>  	}
> >>+	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
> >>
> >>  	/* Add internal object to object list */
> >>  	efi_add_handle(&obj->header);
> >>@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> >>  	}
> >>
> >>  	current_image = image_handle;
> >>+	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
> >>  	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
> >>  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> >>
> >>--
> >>2.20.1
> >>
> >
> 

[U-Boot] [PATCH 4/4] efi_loader: unload applications upon Exit()

[Heinrich Schuchardt]

On 5/7/19 6:39 AM, Takahiro Akashi wrote:
> On Sat, May 04, 2019 at 10:36:36AM +0200, Heinrich Schuchardt wrote:
>> Implement unloading of images in the Exit() boot services:
>>
>> * unload images that are not yet started,
>> * unload started applications,
>> * unload drivers returning an error.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   include/efi_loader.h              |  1 +
>>   lib/efi_loader/efi_boottime.c     | 34 ++++++++++++++++++++++++++-----
>>   lib/efi_loader/efi_image_loader.c |  2 ++
>>   3 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index c2a449e5b6..d73c89ac26 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -237,6 +237,7 @@ struct efi_loaded_image_obj {
>>   	struct jmp_buf_data exit_jmp;
>>   	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
>>   				     struct efi_system_table *st);
>> +	u16 image_type;
>>   };
>>
>>   /**
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index bbcd66caa6..51e0bb2fd5 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/libfdt_env.h>
>>   #include <u-boot/crc.h>
>>   #include <bootm.h>
>> +#include <pe.h>
>>   #include <watchdog.h>
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>> @@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>   	 *	 image protocol.
>>   	 */
>>   	efi_status_t ret;
>> -	void *info;
>> +	struct efi_loaded_image *loaded_image_protocol;
>>   	struct efi_loaded_image_obj *image_obj =
>>   		(struct efi_loaded_image_obj *)image_handle;
>>
>> @@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>   		  exit_data_size, exit_data);
>>
>>   	/* Check parameters */
>> -	if (image_handle != current_image)
>> +	if (image_handle != current_image) {
>
> Does this check make sense even for a driver?

The check is prescribed in the UEFI specification. See the heading
"Status Codes Returned".

Multiple binaries may be in status started. If a child image (which may
be a driver) calls Exit() with the parent image handle this might cause
fancy problems.

The lifetime of a driver is LoadImage(), StartImage(), Exit(),
UnloadImage(). Typically between StartImage() and Exit() it will install
its protocols and between Exit() and UnloadImage() wait for other
binaries to call the protocols.

>
>> +		ret = EFI_INVALID_PARAMETER;
>>   		goto out;
>> +	}
>>   	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
>> -					 &info, NULL, NULL,
>> +					 (void **)&loaded_image_protocol,
>> +					 NULL, NULL,
>>   					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
>> -	if (ret != EFI_SUCCESS)
>> +	if (ret != EFI_SUCCESS) {
>
> Is this check necessary even when 'header.type' is introduced?

I am passing the loaded image protocol to efi_delete_handle(). In
principal a binary might have uninstalled its own loaded image protocol
so this call may fail.

>
>> +		ret = EFI_INVALID_PARAMETER;
>>   		goto out;
>> +	}
>> +
>> +	/* Unloading of unstarted images */
>
> 'Unload' sounds odd when we call efi_delete_image(), doesn't it?
>
>> +	switch (image_obj->header.type) {
>> +	case EFI_OBJECT_TYPE_STARTED_IMAGE:
>> +		break;
>> +	case EFI_OBJECT_TYPE_LOADED_IMAGE:
>> +		efi_delete_image(image_obj, loaded_image_protocol);
>> +		ret = EFI_SUCCESS;
>> +		goto out;
>> +	default:
>> +		/* Handle does not refer to loaded image */
>> +		ret = EFI_INVALID_PARAMETER;
>> +		goto out;
>> +	}
>> +	image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
>>
>>   	/* Exit data is only foreseen in case of failure. */
>>   	if (exit_status != EFI_SUCCESS) {
>> @@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>   		if (ret != EFI_SUCCESS)
>>   			EFI_PRINT("%s: out of memory\n", __func__);
>>   	}
>> +	if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
>> +	    exit_status != EFI_SUCCESS)
>> +		efi_delete_image(image_obj, loaded_image_protocol);
>
> Why not merge those two efi_delete_image()?

I went for readable code. The if statement catching all cases was
unreadable to me.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>>   	/* Make sure entry/exit counts for EFI world cross-overs match */
>>   	EFI_EXIT(exit_status);
>> @@ -2837,7 +2861,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>
>>   	panic("EFI application exited");
>>   out:
>> -	return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +	return EFI_EXIT(ret);
>>   }
>>
>>   /**
>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
>> index f8092b6202..13541cfa7a 100644
>> --- a/lib/efi_loader/efi_image_loader.c
>> +++ b/lib/efi_loader/efi_image_loader.c
>> @@ -273,6 +273,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>   		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
>>   		image_base = opt->ImageBase;
>>   		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
>> +		handle->image_type = opt->Subsystem;
>>   		efi_reloc = efi_alloc(virt_size,
>>   				      loaded_image_info->image_code_type);
>>   		if (!efi_reloc) {
>> @@ -288,6 +289,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>   		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
>>   		image_base = opt->ImageBase;
>>   		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
>> +		handle->image_type = opt->Subsystem;
>>   		efi_reloc = efi_alloc(virt_size,
>>   				      loaded_image_info->image_code_type);
>>   		if (!efi_reloc) {
>> --
>> 2.20.1
>>
>

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Heinrich Schuchardt]

On 5/7/19 7:44 AM, Takahiro Akashi wrote:
> On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
>> On 5/7/19 5:02 AM, Takahiro Akashi wrote:
>>> On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
>>>> In UnloadImage() we need to know if an image is already started.
>>>>
>>>> Add a field to the handle structure identifying loaded and started images.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>   include/efi_loader.h          | 13 +++++++++++++
>>>>   lib/efi_loader/efi_boottime.c |  2 ++
>>>>   2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 3fd9901d66..c2a449e5b6 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -182,6 +182,18 @@ struct efi_handler {
>>>>   	struct list_head open_infos;
>>>>   };
>>>>
>>>> +/**
>>>> + * enum efi_object_type - type of EFI object
>>>> + *
>>>> + * In UnloadImage we must be able to identify if the handle relates to a
>>>> + * started image.
>>>> + */
>>>> +enum efi_object_type {
>>>> +	EFI_OBJECT_TYPE_UNDEFINED = 0,
>>>> +	EFI_OBJECT_TYPE_LOADED_IMAGE,
>>>> +	EFI_OBJECT_TYPE_STARTED_IMAGE,
>>>> +};
>>>
>>> It sounds *status*, not *type*.
>>> In a separate patch, you added U_BOOT_FIRMWARE.
>>> We should distinguish status and type for future enhancement and
>>> to avoid any confusion.
>>
>> Having both information in the same field makes the evaluation easier.
>>
>> Currently I see not necessity for more handle types to be
>> differentiated. Let's change it when the need arises.
>
> I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE
> for root node. Then we can rename efi_object_type to efi_image_status.

This would allow calling UnloadImage() for the root node.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>
>>>>   /**
>>>>    * struct efi_object - dereferenced EFI handle
>>>>    *
>>>> @@ -204,6 +216,7 @@ struct efi_object {
>>>>   	struct list_head link;
>>>>   	/* The list of protocols */
>>>>   	struct list_head protocols;
>>>> +	enum efi_object_type type;
>>>>   };
>>>>
>>>>   /**
>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>> index 3ed08e7c37..dc444fccf6 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>>>>   		free(info);
>>>>   		return EFI_OUT_OF_RESOURCES;
>>>>   	}
>>>> +	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
>>>>
>>>>   	/* Add internal object to object list */
>>>>   	efi_add_handle(&obj->header);
>>>> @@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>>>   	}
>>>>
>>>>   	current_image = image_handle;
>>>> +	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
>>>>   	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
>>>>   	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>>>
>>>> --
>>>> 2.20.1
>>>>
>>>
>>
>

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Takahiro Akashi]

On Tue, May 07, 2019 at 07:53:41AM +0200, Heinrich Schuchardt wrote:
> On 5/7/19 7:44 AM, Takahiro Akashi wrote:
> >On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
> >>On 5/7/19 5:02 AM, Takahiro Akashi wrote:
> >>>On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
> >>>>In UnloadImage() we need to know if an image is already started.
> >>>>
> >>>>Add a field to the handle structure identifying loaded and started images.
> >>>>
> >>>>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>---
> >>>>  include/efi_loader.h          | 13 +++++++++++++
> >>>>  lib/efi_loader/efi_boottime.c |  2 ++
> >>>>  2 files changed, 15 insertions(+)
> >>>>
> >>>>diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>>>index 3fd9901d66..c2a449e5b6 100644
> >>>>--- a/include/efi_loader.h
> >>>>+++ b/include/efi_loader.h
> >>>>@@ -182,6 +182,18 @@ struct efi_handler {
> >>>>  	struct list_head open_infos;
> >>>>  };
> >>>>
> >>>>+/**
> >>>>+ * enum efi_object_type - type of EFI object
> >>>>+ *
> >>>>+ * In UnloadImage we must be able to identify if the handle relates to a
> >>>>+ * started image.
> >>>>+ */
> >>>>+enum efi_object_type {
> >>>>+	EFI_OBJECT_TYPE_UNDEFINED = 0,
> >>>>+	EFI_OBJECT_TYPE_LOADED_IMAGE,
> >>>>+	EFI_OBJECT_TYPE_STARTED_IMAGE,
> >>>>+};
> >>>
> >>>It sounds *status*, not *type*.
> >>>In a separate patch, you added U_BOOT_FIRMWARE.
> >>>We should distinguish status and type for future enhancement and
> >>>to avoid any confusion.
> >>
> >>Having both information in the same field makes the evaluation easier.
> >>
> >>Currently I see not necessity for more handle types to be
> >>differentiated. Let's change it when the need arises.
> >
> >I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE
> >for root node. Then we can rename efi_object_type to efi_image_status.
> 
> This would allow calling UnloadImage() for the root node.

Who knows the address of root node?
For safe guard, you can add
    if (handle == root_node)
        return EFI_INVALID_PARAMETER;
at the beginning of unload_image.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> >-Takahiro Akashi
> >
> >
> >>Best regards
> >>
> >>Heinrich
> >>
> >>>
> >>>Thanks,
> >>>-Takahiro Akashi
> >>>
> >>>
> >>>>  /**
> >>>>   * struct efi_object - dereferenced EFI handle
> >>>>   *
> >>>>@@ -204,6 +216,7 @@ struct efi_object {
> >>>>  	struct list_head link;
> >>>>  	/* The list of protocols */
> >>>>  	struct list_head protocols;
> >>>>+	enum efi_object_type type;
> >>>>  };
> >>>>
> >>>>  /**
> >>>>diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >>>>index 3ed08e7c37..dc444fccf6 100644
> >>>>--- a/lib/efi_loader/efi_boottime.c
> >>>>+++ b/lib/efi_loader/efi_boottime.c
> >>>>@@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >>>>  		free(info);
> >>>>  		return EFI_OUT_OF_RESOURCES;
> >>>>  	}
> >>>>+	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
> >>>>
> >>>>  	/* Add internal object to object list */
> >>>>  	efi_add_handle(&obj->header);
> >>>>@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> >>>>  	}
> >>>>
> >>>>  	current_image = image_handle;
> >>>>+	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
> >>>>  	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
> >>>>  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> >>>>
> >>>>--
> >>>>2.20.1
> >>>>
> >>>
> >>
> >
> 

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Heinrich Schuchardt]

On 5/7/19 7:58 AM, Takahiro Akashi wrote:
> On Tue, May 07, 2019 at 07:53:41AM +0200, Heinrich Schuchardt wrote:
>> On 5/7/19 7:44 AM, Takahiro Akashi wrote:
>>> On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
>>>> On 5/7/19 5:02 AM, Takahiro Akashi wrote:
>>>>> On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
>>>>>> In UnloadImage() we need to know if an image is already started.
>>>>>>
>>>>>> Add a field to the handle structure identifying loaded and started images.
>>>>>>
>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>> ---
>>>>>>   include/efi_loader.h          | 13 +++++++++++++
>>>>>>   lib/efi_loader/efi_boottime.c |  2 ++
>>>>>>   2 files changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>>>> index 3fd9901d66..c2a449e5b6 100644
>>>>>> --- a/include/efi_loader.h
>>>>>> +++ b/include/efi_loader.h
>>>>>> @@ -182,6 +182,18 @@ struct efi_handler {
>>>>>>   	struct list_head open_infos;
>>>>>>   };
>>>>>>
>>>>>> +/**
>>>>>> + * enum efi_object_type - type of EFI object
>>>>>> + *
>>>>>> + * In UnloadImage we must be able to identify if the handle relates to a
>>>>>> + * started image.
>>>>>> + */
>>>>>> +enum efi_object_type {
>>>>>> +	EFI_OBJECT_TYPE_UNDEFINED = 0,
>>>>>> +	EFI_OBJECT_TYPE_LOADED_IMAGE,
>>>>>> +	EFI_OBJECT_TYPE_STARTED_IMAGE,
>>>>>> +};
>>>>>
>>>>> It sounds *status*, not *type*.
>>>>> In a separate patch, you added U_BOOT_FIRMWARE.
>>>>> We should distinguish status and type for future enhancement and
>>>>> to avoid any confusion.
>>>>
>>>> Having both information in the same field makes the evaluation easier.
>>>>
>>>> Currently I see not necessity for more handle types to be
>>>> differentiated. Let's change it when the need arises.
>>>
>>> I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE
>>> for root node. Then we can rename efi_object_type to efi_image_status.
>>
>> This would allow calling UnloadImage() for the root node.
>
> Who knows the address of root node?

Just enumerated handles with a loaded image protocol.

> For safe guard, you can add
>      if (handle == root_node)
>          return EFI_INVALID_PARAMETER;
> at the beginning of unload_image.

This would not decrease complexity.

Regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> Thanks,
>>>>> -Takahiro Akashi
>>>>>
>>>>>
>>>>>>   /**
>>>>>>    * struct efi_object - dereferenced EFI handle
>>>>>>    *
>>>>>> @@ -204,6 +216,7 @@ struct efi_object {
>>>>>>   	struct list_head link;
>>>>>>   	/* The list of protocols */
>>>>>>   	struct list_head protocols;
>>>>>> +	enum efi_object_type type;
>>>>>>   };
>>>>>>
>>>>>>   /**
>>>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>>>> index 3ed08e7c37..dc444fccf6 100644
>>>>>> --- a/lib/efi_loader/efi_boottime.c
>>>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>>>> @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>>>>>>   		free(info);
>>>>>>   		return EFI_OUT_OF_RESOURCES;
>>>>>>   	}
>>>>>> +	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
>>>>>>
>>>>>>   	/* Add internal object to object list */
>>>>>>   	efi_add_handle(&obj->header);
>>>>>> @@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>>>>>   	}
>>>>>>
>>>>>>   	current_image = image_handle;
>>>>>> +	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
>>>>>>   	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
>>>>>>   	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>>>>>
>>>>>> --
>>>>>> 2.20.1
>>>>>>
>>>>>
>>>>
>>>
>>
>

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Takahiro Akashi]

On Tue, May 07, 2019 at 08:05:48AM +0200, Heinrich Schuchardt wrote:
> On 5/7/19 7:58 AM, Takahiro Akashi wrote:
> >On Tue, May 07, 2019 at 07:53:41AM +0200, Heinrich Schuchardt wrote:
> >>On 5/7/19 7:44 AM, Takahiro Akashi wrote:
> >>>On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
> >>>>On 5/7/19 5:02 AM, Takahiro Akashi wrote:
> >>>>>On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
> >>>>>>In UnloadImage() we need to know if an image is already started.
> >>>>>>
> >>>>>>Add a field to the handle structure identifying loaded and started images.
> >>>>>>
> >>>>>>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>>---
> >>>>>>  include/efi_loader.h          | 13 +++++++++++++
> >>>>>>  lib/efi_loader/efi_boottime.c |  2 ++
> >>>>>>  2 files changed, 15 insertions(+)
> >>>>>>
> >>>>>>diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>>>>>index 3fd9901d66..c2a449e5b6 100644
> >>>>>>--- a/include/efi_loader.h
> >>>>>>+++ b/include/efi_loader.h
> >>>>>>@@ -182,6 +182,18 @@ struct efi_handler {
> >>>>>>  	struct list_head open_infos;
> >>>>>>  };
> >>>>>>
> >>>>>>+/**
> >>>>>>+ * enum efi_object_type - type of EFI object
> >>>>>>+ *
> >>>>>>+ * In UnloadImage we must be able to identify if the handle relates to a
> >>>>>>+ * started image.
> >>>>>>+ */
> >>>>>>+enum efi_object_type {
> >>>>>>+	EFI_OBJECT_TYPE_UNDEFINED = 0,
> >>>>>>+	EFI_OBJECT_TYPE_LOADED_IMAGE,
> >>>>>>+	EFI_OBJECT_TYPE_STARTED_IMAGE,
> >>>>>>+};
> >>>>>
> >>>>>It sounds *status*, not *type*.
> >>>>>In a separate patch, you added U_BOOT_FIRMWARE.
> >>>>>We should distinguish status and type for future enhancement and
> >>>>>to avoid any confusion.
> >>>>
> >>>>Having both information in the same field makes the evaluation easier.
> >>>>
> >>>>Currently I see not necessity for more handle types to be
> >>>>differentiated. Let's change it when the need arises.
> >>>
> >>>I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE
> >>>for root node. Then we can rename efi_object_type to efi_image_status.
> >>
> >>This would allow calling UnloadImage() for the root node.
> >
> >Who knows the address of root node?
> 
> Just enumerated handles with a loaded image protocol.

Remove it when enumerating handles if necessary.


> >For safe guard, you can add
> >     if (handle == root_node)
> >         return EFI_INVALID_PARAMETER;
> >at the beginning of unload_image.
> 
> This would not decrease complexity.

I believe that it's much better and more understandable than
confusing use of status and type.

-Takahiro Akashi


> Regards
> 
> Heinrich
> 
> >
> >-Takahiro Akashi
> >
> >
> >>Best regards
> >>
> >>Heinrich
> >>
> >>>
> >>>-Takahiro Akashi
> >>>
> >>>
> >>>>Best regards
> >>>>
> >>>>Heinrich
> >>>>
> >>>>>
> >>>>>Thanks,
> >>>>>-Takahiro Akashi
> >>>>>
> >>>>>
> >>>>>>  /**
> >>>>>>   * struct efi_object - dereferenced EFI handle
> >>>>>>   *
> >>>>>>@@ -204,6 +216,7 @@ struct efi_object {
> >>>>>>  	struct list_head link;
> >>>>>>  	/* The list of protocols */
> >>>>>>  	struct list_head protocols;
> >>>>>>+	enum efi_object_type type;
> >>>>>>  };
> >>>>>>
> >>>>>>  /**
> >>>>>>diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >>>>>>index 3ed08e7c37..dc444fccf6 100644
> >>>>>>--- a/lib/efi_loader/efi_boottime.c
> >>>>>>+++ b/lib/efi_loader/efi_boottime.c
> >>>>>>@@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >>>>>>  		free(info);
> >>>>>>  		return EFI_OUT_OF_RESOURCES;
> >>>>>>  	}
> >>>>>>+	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
> >>>>>>
> >>>>>>  	/* Add internal object to object list */
> >>>>>>  	efi_add_handle(&obj->header);
> >>>>>>@@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> >>>>>>  	}
> >>>>>>
> >>>>>>  	current_image = image_handle;
> >>>>>>+	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
> >>>>>>  	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
> >>>>>>  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> >>>>>>
> >>>>>>--
> >>>>>>2.20.1
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
> 

[U-Boot] [PATCH 4/4] efi_loader: unload applications upon Exit()

[Takahiro Akashi]

On Tue, May 07, 2019 at 07:50:48AM +0200, Heinrich Schuchardt wrote:
> On 5/7/19 6:39 AM, Takahiro Akashi wrote:
> >On Sat, May 04, 2019 at 10:36:36AM +0200, Heinrich Schuchardt wrote:
> >>Implement unloading of images in the Exit() boot services:
> >>
> >>* unload images that are not yet started,
> >>* unload started applications,
> >>* unload drivers returning an error.
> >>
> >>Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>---
> >>  include/efi_loader.h              |  1 +
> >>  lib/efi_loader/efi_boottime.c     | 34 ++++++++++++++++++++++++++-----
> >>  lib/efi_loader/efi_image_loader.c |  2 ++
> >>  3 files changed, 32 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>index c2a449e5b6..d73c89ac26 100644
> >>--- a/include/efi_loader.h
> >>+++ b/include/efi_loader.h
> >>@@ -237,6 +237,7 @@ struct efi_loaded_image_obj {
> >>  	struct jmp_buf_data exit_jmp;
> >>  	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
> >>  				     struct efi_system_table *st);
> >>+	u16 image_type;
> >>  };
> >>
> >>  /**
> >>diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >>index bbcd66caa6..51e0bb2fd5 100644
> >>--- a/lib/efi_loader/efi_boottime.c
> >>+++ b/lib/efi_loader/efi_boottime.c
> >>@@ -13,6 +13,7 @@
> >>  #include <linux/libfdt_env.h>
> >>  #include <u-boot/crc.h>
> >>  #include <bootm.h>
> >>+#include <pe.h>
> >>  #include <watchdog.h>
> >>
> >>  DECLARE_GLOBAL_DATA_PTR;
> >>@@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >>  	 *	 image protocol.
> >>  	 */
> >>  	efi_status_t ret;
> >>-	void *info;
> >>+	struct efi_loaded_image *loaded_image_protocol;
> >>  	struct efi_loaded_image_obj *image_obj =
> >>  		(struct efi_loaded_image_obj *)image_handle;
> >>
> >>@@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >>  		  exit_data_size, exit_data);
> >>
> >>  	/* Check parameters */
> >>-	if (image_handle != current_image)
> >>+	if (image_handle != current_image) {
> >
> >Does this check make sense even for a driver?
> 
> The check is prescribed in the UEFI specification. See the heading
> "Status Codes Returned".
> 
> Multiple binaries may be in status started. If a child image (which may
> be a driver) calls Exit() with the parent image handle this might cause
> fancy problems.
> 
> The lifetime of a driver is LoadImage(), StartImage(), Exit(),
> UnloadImage(). Typically between StartImage() and Exit() it will install
> its protocols and between Exit() and UnloadImage() wait for other
> binaries to call the protocols.

If this is true, that will be fine for a driver.
But what about 'not started' application? In "Status Codes Returned" of
Exit(), it says
  EFI_SUCCESS
  The image specified by ImageHandle was unloaded. This condition only
  occurs for images that have been loaded with LoadImage() but have not
  been started with StartImage().
In this case, the caller of Exit() is not an application.

> >
> >>+		ret = EFI_INVALID_PARAMETER;
> >>  		goto out;
> >>+	}
> >>  	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
> >>-					 &info, NULL, NULL,
> >>+					 (void **)&loaded_image_protocol,
> >>+					 NULL, NULL,
> >>  					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
> >>-	if (ret != EFI_SUCCESS)
> >>+	if (ret != EFI_SUCCESS) {
> >
> >Is this check necessary even when 'header.type' is introduced?
> 
> I am passing the loaded image protocol to efi_delete_handle().

I don't think that it can be a critical reason.

> In
> principal a binary might have uninstalled its own loaded image protocol
> so this call may fail.

If we admit this possibility, there will be bunch of fragile code elsewhere.

> >
> >>+		ret = EFI_INVALID_PARAMETER;
> >>  		goto out;
> >>+	}
> >>+
> >>+	/* Unloading of unstarted images */
> >
> >'Unload' sounds odd when we call efi_delete_image(), doesn't it?
> >
> >>+	switch (image_obj->header.type) {
> >>+	case EFI_OBJECT_TYPE_STARTED_IMAGE:
> >>+		break;
> >>+	case EFI_OBJECT_TYPE_LOADED_IMAGE:
> >>+		efi_delete_image(image_obj, loaded_image_protocol);
> >>+		ret = EFI_SUCCESS;
> >>+		goto out;
> >>+	default:
> >>+		/* Handle does not refer to loaded image */
> >>+		ret = EFI_INVALID_PARAMETER;
> >>+		goto out;
> >>+	}
> >>+	image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
> >>
> >>  	/* Exit data is only foreseen in case of failure. */
> >>  	if (exit_status != EFI_SUCCESS) {
> >>@@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >>  		if (ret != EFI_SUCCESS)
> >>  			EFI_PRINT("%s: out of memory\n", __func__);
> >>  	}
> >>+	if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
> >>+	    exit_status != EFI_SUCCESS)
> >>+		efi_delete_image(image_obj, loaded_image_protocol);
> >
> >Why not merge those two efi_delete_image()?
> 
> I went for readable code. The if statement catching all cases was
> unreadable to me.

For me, your code is much unreadable.
Moreover, I remember that you have said, in a review of my patch, that
we should use "goto" only in error cases.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> >
> >-Takahiro Akashi
> >
> >
> >>  	/* Make sure entry/exit counts for EFI world cross-overs match */
> >>  	EFI_EXIT(exit_status);
> >>@@ -2837,7 +2861,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
> >>
> >>  	panic("EFI application exited");
> >>  out:
> >>-	return EFI_EXIT(EFI_INVALID_PARAMETER);
> >>+	return EFI_EXIT(ret);
> >>  }
> >>
> >>  /**
> >>diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> >>index f8092b6202..13541cfa7a 100644
> >>--- a/lib/efi_loader/efi_image_loader.c
> >>+++ b/lib/efi_loader/efi_image_loader.c
> >>@@ -273,6 +273,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
> >>  		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
> >>  		image_base = opt->ImageBase;
> >>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> >>+		handle->image_type = opt->Subsystem;
> >>  		efi_reloc = efi_alloc(virt_size,
> >>  				      loaded_image_info->image_code_type);
> >>  		if (!efi_reloc) {
> >>@@ -288,6 +289,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
> >>  		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
> >>  		image_base = opt->ImageBase;
> >>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> >>+		handle->image_type = opt->Subsystem;
> >>  		efi_reloc = efi_alloc(virt_size,
> >>  				      loaded_image_info->image_code_type);
> >>  		if (!efi_reloc) {
> >>--
> >>2.20.1
> >>
> >
> 

[U-Boot] [PATCH 4/4] efi_loader: unload applications upon Exit()

[Heinrich Schuchardt]

On 5/7/19 8:37 AM, Takahiro Akashi wrote:
> On Tue, May 07, 2019 at 07:50:48AM +0200, Heinrich Schuchardt wrote:
>> On 5/7/19 6:39 AM, Takahiro Akashi wrote:
>>> On Sat, May 04, 2019 at 10:36:36AM +0200, Heinrich Schuchardt wrote:
>>>> Implement unloading of images in the Exit() boot services:
>>>>
>>>> * unload images that are not yet started,
>>>> * unload started applications,
>>>> * unload drivers returning an error.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>  include/efi_loader.h              |  1 +
>>>>  lib/efi_loader/efi_boottime.c     | 34 ++++++++++++++++++++++++++-----
>>>>  lib/efi_loader/efi_image_loader.c |  2 ++
>>>>  3 files changed, 32 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index c2a449e5b6..d73c89ac26 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -237,6 +237,7 @@ struct efi_loaded_image_obj {
>>>>  	struct jmp_buf_data exit_jmp;
>>>>  	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
>>>>  				     struct efi_system_table *st);
>>>> +	u16 image_type;
>>>>  };
>>>>
>>>>  /**
>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>> index bbcd66caa6..51e0bb2fd5 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -13,6 +13,7 @@
>>>>  #include <linux/libfdt_env.h>
>>>>  #include <u-boot/crc.h>
>>>>  #include <bootm.h>
>>>> +#include <pe.h>
>>>>  #include <watchdog.h>
>>>>
>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>> @@ -2798,7 +2799,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>>>  	 *	 image protocol.
>>>>  	 */
>>>>  	efi_status_t ret;
>>>> -	void *info;
>>>> +	struct efi_loaded_image *loaded_image_protocol;
>>>>  	struct efi_loaded_image_obj *image_obj =
>>>>  		(struct efi_loaded_image_obj *)image_handle;
>>>>
>>>> @@ -2806,13 +2807,33 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>>>  		  exit_data_size, exit_data);
>>>>
>>>>  	/* Check parameters */
>>>> -	if (image_handle != current_image)
>>>> +	if (image_handle != current_image) {
>>>
>>> Does this check make sense even for a driver?
>>
>> The check is prescribed in the UEFI specification. See the heading
>> "Status Codes Returned".
>>
>> Multiple binaries may be in status started. If a child image (which may
>> be a driver) calls Exit() with the parent image handle this might cause
>> fancy problems.
>>
>> The lifetime of a driver is LoadImage(), StartImage(), Exit(),
>> UnloadImage(). Typically between StartImage() and Exit() it will install
>> its protocols and between Exit() and UnloadImage() wait for other
>> binaries to call the protocols.
>
> If this is true, that will be fine for a driver.
> But what about 'not started' application? In "Status Codes Returned" of
> Exit(), it says
>   EFI_SUCCESS
>   The image specified by ImageHandle was unloaded. This condition only
>   occurs for images that have been loaded with LoadImage() but have not
>   been started with StartImage().
> In this case, the caller of Exit() is not an application.
>
>>>
>>>> +		ret = EFI_INVALID_PARAMETER;
>>>>  		goto out;
>>>> +	}
>>>>  	ret = EFI_CALL(efi_open_protocol(image_handle, &efi_guid_loaded_image,
>>>> -					 &info, NULL, NULL,
>>>> +					 (void **)&loaded_image_protocol,
>>>> +					 NULL, NULL,
>>>>  					 EFI_OPEN_PROTOCOL_GET_PROTOCOL));
>>>> -	if (ret != EFI_SUCCESS)
>>>> +	if (ret != EFI_SUCCESS) {
>>>
>>> Is this check necessary even when 'header.type' is introduced?
>>
>> I am passing the loaded image protocol to efi_delete_handle().
>
> I don't think that it can be a critical reason.
>
>> In
>> principal a binary might have uninstalled its own loaded image protocol
>> so this call may fail.
>
> If we admit this possibility, there will be bunch of fragile code elsewhere.
>
>>>
>>>> +		ret = EFI_INVALID_PARAMETER;
>>>>  		goto out;
>>>> +	}
>>>> +
>>>> +	/* Unloading of unstarted images */
>>>
>>> 'Unload' sounds odd when we call efi_delete_image(), doesn't it?
>>>
>>>> +	switch (image_obj->header.type) {
>>>> +	case EFI_OBJECT_TYPE_STARTED_IMAGE:
>>>> +		break;
>>>> +	case EFI_OBJECT_TYPE_LOADED_IMAGE:
>>>> +		efi_delete_image(image_obj, loaded_image_protocol);
>>>> +		ret = EFI_SUCCESS;
>>>> +		goto out;
>>>> +	default:
>>>> +		/* Handle does not refer to loaded image */
>>>> +		ret = EFI_INVALID_PARAMETER;
>>>> +		goto out;
>>>> +	}
>>>> +	image_obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
>>>>
>>>>  	/* Exit data is only foreseen in case of failure. */
>>>>  	if (exit_status != EFI_SUCCESS) {
>>>> @@ -2822,6 +2843,9 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>>>  		if (ret != EFI_SUCCESS)
>>>>  			EFI_PRINT("%s: out of memory\n", __func__);
>>>>  	}
>>>> +	if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION ||
>>>> +	    exit_status != EFI_SUCCESS)
>>>> +		efi_delete_image(image_obj, loaded_image_protocol);
>>>
>>> Why not merge those two efi_delete_image()?
>>
>> I went for readable code. The if statement catching all cases was
>> unreadable to me.
>
> For me, your code is much unreadable.
> Moreover, I remember that you have said, in a review of my patch, that
> we should use "goto" only in error cases.

Good point. So the check must be after handling
EFI_OBJECT_TYPE_LOADED_IMAGE.

I will revise the patch.

Thanks for reviewing.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>>  	/* Make sure entry/exit counts for EFI world cross-overs match */
>>>>  	EFI_EXIT(exit_status);
>>>> @@ -2837,7 +2861,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>>>
>>>>  	panic("EFI application exited");
>>>>  out:
>>>> -	return EFI_EXIT(EFI_INVALID_PARAMETER);
>>>> +	return EFI_EXIT(ret);
>>>>  }
>>>>
>>>>  /**
>>>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
>>>> index f8092b6202..13541cfa7a 100644
>>>> --- a/lib/efi_loader/efi_image_loader.c
>>>> +++ b/lib/efi_loader/efi_image_loader.c
>>>> @@ -273,6 +273,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>>  		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
>>>>  		image_base = opt->ImageBase;
>>>>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
>>>> +		handle->image_type = opt->Subsystem;
>>>>  		efi_reloc = efi_alloc(virt_size,
>>>>  				      loaded_image_info->image_code_type);
>>>>  		if (!efi_reloc) {
>>>> @@ -288,6 +289,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>>  		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
>>>>  		image_base = opt->ImageBase;
>>>>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
>>>> +		handle->image_type = opt->Subsystem;
>>>>  		efi_reloc = efi_alloc(virt_size,
>>>>  				      loaded_image_info->image_code_type);
>>>>  		if (!efi_reloc) {
>>>> --
>>>> 2.20.1
>>>>
>>>
>>
>

[U-Boot] [PATCH 1/4] efi_loader: mark started images

[Heinrich Schuchardt]

On 5/7/19 8:12 AM, Takahiro Akashi wrote:
> On Tue, May 07, 2019 at 08:05:48AM +0200, Heinrich Schuchardt wrote:
>> On 5/7/19 7:58 AM, Takahiro Akashi wrote:
>>> On Tue, May 07, 2019 at 07:53:41AM +0200, Heinrich Schuchardt wrote:
>>>> On 5/7/19 7:44 AM, Takahiro Akashi wrote:
>>>>> On Tue, May 07, 2019 at 07:26:46AM +0200, Heinrich Schuchardt wrote:
>>>>>> On 5/7/19 5:02 AM, Takahiro Akashi wrote:
>>>>>>> On Sat, May 04, 2019 at 10:36:33AM +0200, Heinrich Schuchardt wrote:
>>>>>>>> In UnloadImage() we need to know if an image is already started.
>>>>>>>>
>>>>>>>> Add a field to the handle structure identifying loaded and started images.
>>>>>>>>
>>>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>>>> ---
>>>>>>>>  include/efi_loader.h          | 13 +++++++++++++
>>>>>>>>  lib/efi_loader/efi_boottime.c |  2 ++
>>>>>>>>  2 files changed, 15 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>>>>>> index 3fd9901d66..c2a449e5b6 100644
>>>>>>>> --- a/include/efi_loader.h
>>>>>>>> +++ b/include/efi_loader.h
>>>>>>>> @@ -182,6 +182,18 @@ struct efi_handler {
>>>>>>>>  	struct list_head open_infos;
>>>>>>>>  };
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * enum efi_object_type - type of EFI object
>>>>>>>> + *
>>>>>>>> + * In UnloadImage we must be able to identify if the handle relates to a
>>>>>>>> + * started image.
>>>>>>>> + */
>>>>>>>> +enum efi_object_type {
>>>>>>>> +	EFI_OBJECT_TYPE_UNDEFINED = 0,
>>>>>>>> +	EFI_OBJECT_TYPE_LOADED_IMAGE,
>>>>>>>> +	EFI_OBJECT_TYPE_STARTED_IMAGE,
>>>>>>>> +};
>>>>>>>
>>>>>>> It sounds *status*, not *type*.
>>>>>>> In a separate patch, you added U_BOOT_FIRMWARE.
>>>>>>> We should distinguish status and type for future enhancement and
>>>>>>> to avoid any confusion.
>>>>>>
>>>>>> Having both information in the same field makes the evaluation easier.
>>>>>>
>>>>>> Currently I see not necessity for more handle types to be
>>>>>> differentiated. Let's change it when the need arises.
>>>>>
>>>>> I don't think we need U_BOOT_FIRMWARE if we use STARTED_IMAGE
>>>>> for root node. Then we can rename efi_object_type to efi_image_status.
>>>>
>>>> This would allow calling UnloadImage() for the root node.
>>>
>>> Who knows the address of root node?
>>
>> Just enumerated handles with a loaded image protocol.
>
> Remove it when enumerating handles if necessary.

No, we have to return the handle when LocateHandle() is called for the
loaded image protocol.

Regards

Heinrich

>
>
>>> For safe guard, you can add
>>>     if (handle == root_node)
>>>         return EFI_INVALID_PARAMETER;
>>> at the beginning of unload_image.
>>
>> This would not decrease complexity.
>
> I believe that it's much better and more understandable than
> confusing use of status and type.
>
> -Takahiro Akashi
>
>
>> Regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> -Takahiro Akashi
>>>>>
>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Heinrich
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> -Takahiro Akashi
>>>>>>>
>>>>>>>
>>>>>>>>  /**
>>>>>>>>   * struct efi_object - dereferenced EFI handle
>>>>>>>>   *
>>>>>>>> @@ -204,6 +216,7 @@ struct efi_object {
>>>>>>>>  	struct list_head link;
>>>>>>>>  	/* The list of protocols */
>>>>>>>>  	struct list_head protocols;
>>>>>>>> +	enum efi_object_type type;
>>>>>>>>  };
>>>>>>>>
>>>>>>>>  /**
>>>>>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>>>>>> index 3ed08e7c37..dc444fccf6 100644
>>>>>>>> --- a/lib/efi_loader/efi_boottime.c
>>>>>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>>>>>> @@ -1554,6 +1554,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>>>>>>>>  		free(info);
>>>>>>>>  		return EFI_OUT_OF_RESOURCES;
>>>>>>>>  	}
>>>>>>>> +	obj->header.type = EFI_OBJECT_TYPE_LOADED_IMAGE;
>>>>>>>>
>>>>>>>>  	/* Add internal object to object list */
>>>>>>>>  	efi_add_handle(&obj->header);
>>>>>>>> @@ -2678,6 +2679,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>>>>>>>  	}
>>>>>>>>
>>>>>>>>  	current_image = image_handle;
>>>>>>>> +	image_obj->header.type = EFI_OBJECT_TYPE_STARTED_IMAGE;
>>>>>>>>  	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
>>>>>>>>  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.20.1
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>