diff mbox series

[U-Boot,RFC,v2,01/11] efi_loader: boottime: add loaded image device path protocol to image handle

Message ID 20190327044042.13707-2-takahiro.akashi@linaro.org
State Accepted
Commit bc8fc32855d27b2999ed6667af10123f341b3159
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: rework bootefi/bootmgr | expand

Commit Message

AKASHI Takahiro March 27, 2019, 4:40 a.m. UTC
To meet UEFI spec v2.7a section 9.2, we should add
EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle,
instead of EFI_DEVICE_PATH_PROTOCOL.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_api.h                 |  4 ++++
 include/efi_loader.h              |  1 +
 lib/efi_loader/efi_boottime.c     | 19 ++++++++++++-------
 lib/efi_loader/efi_image_loader.c |  2 ++
 4 files changed, 19 insertions(+), 7 deletions(-)

Comments

Heinrich Schuchardt March 27, 2019, 5:58 a.m. UTC | #1
On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> To meet UEFI spec v2.7a section 9.2, we should add
> EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle,
> instead of EFI_DEVICE_PATH_PROTOCOL.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/efi_api.h                 |  4 ++++
>  include/efi_loader.h              |  1 +
>  lib/efi_loader/efi_boottime.c     | 19 ++++++++++++-------
>  lib/efi_loader/efi_image_loader.c |  2 ++
>  4 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/efi_api.h b/include/efi_api.h
> index ccf608653d4a..63b703c951a7 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -333,6 +333,10 @@ struct efi_system_table {
>  	EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
>  		 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
>
> +#define LOADED_IMAGE_DEVICE_PATH_GUID \
> +	EFI_GUID(0xbc62157e, 0x3e33, 0x4fec, \
> +		 0x99, 0x20, 0x2d, 0x3b, 0x36, 0xd7, 0x50, 0xdf)
> +
>  #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000
>
>  struct efi_loaded_image {
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 512880ab8fbf..3f54e354bdcd 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -135,6 +135,7 @@ extern const efi_guid_t efi_guid_event_group_reset_system;
>  /* GUID of the device tree table */
>  extern const efi_guid_t efi_guid_fdt;
>  extern const efi_guid_t efi_guid_loaded_image;
> +extern const efi_guid_t efi_guid_loaded_image_device_path;
>  extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>  extern const efi_guid_t efi_simple_file_system_protocol_guid;
>  extern const efi_guid_t efi_file_info_guid;
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 391681260c27..578b32a05ffd 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1519,6 +1519,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>  	efi_status_t ret;
>  	struct efi_loaded_image *info = NULL;
>  	struct efi_loaded_image_obj *obj = NULL;
> +	struct efi_device_path *dp;
>
>  	/* In case of EFI_OUT_OF_RESOURCES avoid illegal free by caller. */
>  	*handle_ptr = NULL;
> @@ -1542,15 +1543,19 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>
>  	if (device_path) {
>  		info->device_handle = efi_dp_find_obj(device_path, NULL);
> -		/*
> -		 * When asking for the device path interface, return
> -		 * bootefi_device_path
> -		 */
> -		ret = efi_add_protocol(&obj->header,
> -				       &efi_guid_device_path, device_path);
> -		if (ret != EFI_SUCCESS)
> +
> +		dp = efi_dp_append(device_path, file_path);

I think we should pass the original device_path as a parameter to
efi_setup_loaded_image instead of first splitting and then recombining
devicepath elements.

But let's leave this as a TODO after the rest of the refactoring.

> +		if (!dp) {
> +			ret = EFI_OUT_OF_RESOURCES;
>  			goto failure;
> +		}
> +	} else {
> +		dp = NULL;
>  	}
> +	ret = efi_add_protocol(&obj->header,
> +			       &efi_guid_loaded_image_device_path, dp);

I wondered about the loaded imaged device path protocol possibly being
NULL. But the spec explicitely states:

It is legal to call LoadImage() for a buffer in memory with a NULL
DevicePath parameter. In this case, the Loaded Image Device Path
Protocol is installed with a NULL interface pointer.

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

> +	if (ret != EFI_SUCCESS)
> +		goto failure;
>
>  	/*
>  	 * When asking for the loaded_image interface, just
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index fe66e7b9ffe1..c6177b9ff832 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -14,6 +14,8 @@
>  const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
>  const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
>  const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
> +const efi_guid_t efi_guid_loaded_image_device_path
> +		= LOADED_IMAGE_DEVICE_PATH_GUID;
>  const efi_guid_t efi_simple_file_system_protocol_guid =
>  		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
>  const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;
>
Heinrich Schuchardt March 27, 2019, 9:25 p.m. UTC | #2
On 3/27/19 6:58 AM, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
>> To meet UEFI spec v2.7a section 9.2, we should add
>> EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle,
>> instead of EFI_DEVICE_PATH_PROTOCOL.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---

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

Added to for efi-2019-07 branch.
AKASHI Takahiro March 28, 2019, 1:13 a.m. UTC | #3
On Wed, Mar 27, 2019 at 06:58:31AM +0100, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> > To meet UEFI spec v2.7a section 9.2, we should add
> > EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle,
> > instead of EFI_DEVICE_PATH_PROTOCOL.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/efi_api.h                 |  4 ++++
> >  include/efi_loader.h              |  1 +
> >  lib/efi_loader/efi_boottime.c     | 19 ++++++++++++-------
> >  lib/efi_loader/efi_image_loader.c |  2 ++
> >  4 files changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index ccf608653d4a..63b703c951a7 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -333,6 +333,10 @@ struct efi_system_table {
> >  	EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
> >  		 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
> >
> > +#define LOADED_IMAGE_DEVICE_PATH_GUID \
> > +	EFI_GUID(0xbc62157e, 0x3e33, 0x4fec, \
> > +		 0x99, 0x20, 0x2d, 0x3b, 0x36, 0xd7, 0x50, 0xdf)
> > +
> >  #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000
> >
> >  struct efi_loaded_image {
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 512880ab8fbf..3f54e354bdcd 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -135,6 +135,7 @@ extern const efi_guid_t efi_guid_event_group_reset_system;
> >  /* GUID of the device tree table */
> >  extern const efi_guid_t efi_guid_fdt;
> >  extern const efi_guid_t efi_guid_loaded_image;
> > +extern const efi_guid_t efi_guid_loaded_image_device_path;
> >  extern const efi_guid_t efi_guid_device_path_to_text_protocol;
> >  extern const efi_guid_t efi_simple_file_system_protocol_guid;
> >  extern const efi_guid_t efi_file_info_guid;
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 391681260c27..578b32a05ffd 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1519,6 +1519,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >  	efi_status_t ret;
> >  	struct efi_loaded_image *info = NULL;
> >  	struct efi_loaded_image_obj *obj = NULL;
> > +	struct efi_device_path *dp;
> >
> >  	/* In case of EFI_OUT_OF_RESOURCES avoid illegal free by caller. */
> >  	*handle_ptr = NULL;
> > @@ -1542,15 +1543,19 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >
> >  	if (device_path) {
> >  		info->device_handle = efi_dp_find_obj(device_path, NULL);
> > -		/*
> > -		 * When asking for the device path interface, return
> > -		 * bootefi_device_path
> > -		 */
> > -		ret = efi_add_protocol(&obj->header,
> > -				       &efi_guid_device_path, device_path);
> > -		if (ret != EFI_SUCCESS)
> > +
> > +		dp = efi_dp_append(device_path, file_path);
> 
> I think we should pass the original device_path as a parameter to
> efi_setup_loaded_image instead of first splitting and then recombining
> devicepath elements.

Agree, but I didn't modify the code because efi_setup_loaded_image()
is still used to load "efi_selftest" in bootefi.

I hope that you will work on do_efi_selftest().

> But let's leave this as a TODO after the rest of the refactoring.
> 
> > +		if (!dp) {
> > +			ret = EFI_OUT_OF_RESOURCES;
> >  			goto failure;
> > +		}
> > +	} else {
> > +		dp = NULL;
> >  	}
> > +	ret = efi_add_protocol(&obj->header,
> > +			       &efi_guid_loaded_image_device_path, dp);
> 
> I wondered about the loaded imaged device path protocol possibly being
> NULL. But the spec explicitely states:
> 
> It is legal to call LoadImage() for a buffer in memory with a NULL
> DevicePath parameter. In this case, the Loaded Image Device Path
> Protocol is installed with a NULL interface pointer.

Right, I have also found this statement.

-Takahiro Akashi

> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> > +	if (ret != EFI_SUCCESS)
> > +		goto failure;
> >
> >  	/*
> >  	 * When asking for the loaded_image interface, just
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index fe66e7b9ffe1..c6177b9ff832 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -14,6 +14,8 @@
> >  const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
> >  const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
> >  const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
> > +const efi_guid_t efi_guid_loaded_image_device_path
> > +		= LOADED_IMAGE_DEVICE_PATH_GUID;
> >  const efi_guid_t efi_simple_file_system_protocol_guid =
> >  		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
> >  const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;
> >
>
diff mbox series

Patch

diff --git a/include/efi_api.h b/include/efi_api.h
index ccf608653d4a..63b703c951a7 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -333,6 +333,10 @@  struct efi_system_table {
 	EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
 		 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 
+#define LOADED_IMAGE_DEVICE_PATH_GUID \
+	EFI_GUID(0xbc62157e, 0x3e33, 0x4fec, \
+		 0x99, 0x20, 0x2d, 0x3b, 0x36, 0xd7, 0x50, 0xdf)
+
 #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000
 
 struct efi_loaded_image {
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 512880ab8fbf..3f54e354bdcd 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -135,6 +135,7 @@  extern const efi_guid_t efi_guid_event_group_reset_system;
 /* GUID of the device tree table */
 extern const efi_guid_t efi_guid_fdt;
 extern const efi_guid_t efi_guid_loaded_image;
+extern const efi_guid_t efi_guid_loaded_image_device_path;
 extern const efi_guid_t efi_guid_device_path_to_text_protocol;
 extern const efi_guid_t efi_simple_file_system_protocol_guid;
 extern const efi_guid_t efi_file_info_guid;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 391681260c27..578b32a05ffd 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1519,6 +1519,7 @@  efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
 	efi_status_t ret;
 	struct efi_loaded_image *info = NULL;
 	struct efi_loaded_image_obj *obj = NULL;
+	struct efi_device_path *dp;
 
 	/* In case of EFI_OUT_OF_RESOURCES avoid illegal free by caller. */
 	*handle_ptr = NULL;
@@ -1542,15 +1543,19 @@  efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
 
 	if (device_path) {
 		info->device_handle = efi_dp_find_obj(device_path, NULL);
-		/*
-		 * When asking for the device path interface, return
-		 * bootefi_device_path
-		 */
-		ret = efi_add_protocol(&obj->header,
-				       &efi_guid_device_path, device_path);
-		if (ret != EFI_SUCCESS)
+
+		dp = efi_dp_append(device_path, file_path);
+		if (!dp) {
+			ret = EFI_OUT_OF_RESOURCES;
 			goto failure;
+		}
+	} else {
+		dp = NULL;
 	}
+	ret = efi_add_protocol(&obj->header,
+			       &efi_guid_loaded_image_device_path, dp);
+	if (ret != EFI_SUCCESS)
+		goto failure;
 
 	/*
 	 * When asking for the loaded_image interface, just
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index fe66e7b9ffe1..c6177b9ff832 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -14,6 +14,8 @@ 
 const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
 const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
 const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
+const efi_guid_t efi_guid_loaded_image_device_path
+		= LOADED_IMAGE_DEVICE_PATH_GUID;
 const efi_guid_t efi_simple_file_system_protocol_guid =
 		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
 const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;