[PATCH] efi_loader: fix a problem in loading an image from a short-path

[PATCH] efi_loader: fix a problem in loading an image from a short-path

[AKASHI Takahiro]

Booting from a short-form device path which starts with the first element
being a File Path Media Device Path failed because it doesn't contain
any valid device with simple file system protocol and efi_dp_find_obj()
in efi_load_image_from_path() will return NULL.
For instance,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/\helloworld.efi
  -> shortened version: /\helloworld.efi

With this patch applied, all the media devices with simple file system
protocol are enumerated and the boot manager attempts to boot temporarily
generated device paths one-by-one.

This new implementation is still a bit incompatible with the UEFI
specification in terms of:
* not creating real boot options
* not distinguishing removable media and fix media
(See section 3.1.3 "Boot Options".)

But it still gives us a closer and better solution than the current.

Fixes: commit 9cdf470274ff ("efi_loader: support booting via short-form device-path")
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h          |  3 ++
 lib/efi_loader/efi_boottime.c | 87 +++++++++++++++++++++++++++++++----
 lib/efi_loader/efi_file.c     | 35 ++++++++++----
 3 files changed, 107 insertions(+), 18 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index ba79a9afb404..9730c1375a55 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -661,6 +661,9 @@ void efi_signal_event(struct efi_event *event);
 struct efi_simple_file_system_protocol *efi_simple_file_system(
 		struct blk_desc *desc, int part, struct efi_device_path *dp);
 
+/* open file from simple file system */
+struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,
+					 struct efi_device_path *fp);
 /* open file from device-path: */
 struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp);
 
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 5bcb8253edba..39b0e8f7ade0 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1868,19 +1868,21 @@ out:
 }
 
 /**
- * efi_load_image_from_file() - load an image from file system
+ * __efi_load_image_from_file() - load an image from file system
  *
  * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
  * callers obligation to update the memory type as needed.
  *
+ * @v:			simple file system
  * @file_path:		the path of the image to load
  * @buffer:		buffer containing the loaded image
  * @size:		size of the loaded image
  * Return:		status code
  */
 static
-efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
-				      void **buffer, efi_uintn_t *size)
+efi_status_t __efi_load_image_from_file(struct efi_simple_file_system_protocol *v,
+					struct efi_device_path *file_path,
+					void **buffer, efi_uintn_t *size)
 {
 	struct efi_file_handle *f;
 	efi_status_t ret;
@@ -1888,7 +1890,11 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
 	efi_uintn_t bs;
 
 	/* Open file */
-	f = efi_file_from_path(file_path);
+	if (v)
+		f = efi_file_from_fs(v, file_path);
+	else
+		/* file_path should have a device path */
+		f = efi_file_from_path(file_path);
 	if (!f)
 		return EFI_NOT_FOUND;
 
@@ -1921,6 +1927,64 @@ error:
 	return ret;
 }
 
+/**
+ * efi_load_image_from_file() - load an image from file system
+ *
+ * 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
+ */
+static
+efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
+				      void **buffer, efi_uintn_t *size)
+{
+	efi_uintn_t no_handles;
+	efi_handle_t *handles;
+	struct efi_handler *handler;
+	struct efi_simple_file_system_protocol *fs;
+	int i;
+	efi_status_t ret;
+
+	/* if a file_path contains a device path */
+	if (!EFI_DP_TYPE(file_path, MEDIA_DEVICE, FILE_PATH))
+		return __efi_load_image_from_file(NULL, file_path, buffer, size);
+
+	/* no explicit device specified */
+	ret = EFI_CALL(efi_locate_handle_buffer(
+					BY_PROTOCOL,
+					&efi_simple_file_system_protocol_guid,
+					NULL,
+					&no_handles,
+					&handles));
+	if (ret != EFI_SUCCESS)
+		return ret;
+	if (!no_handles)
+		return EFI_NOT_FOUND;
+
+	for (i = 0; i < no_handles; i++) {
+		ret = efi_search_protocol(handles[i],
+					  &efi_simple_file_system_protocol_guid,
+					  &handler);
+		if (ret != EFI_SUCCESS)
+			/* unlikely */
+			continue;
+
+		fs = handler->protocol_interface;
+		if (!fs)
+			continue;
+
+		ret = __efi_load_image_from_file(fs, file_path, buffer, size);
+		if (ret == EFI_SUCCESS)
+			return ret;
+	}
+
+	return EFI_NOT_FOUND;
+}
+
 /**
  * efi_load_image_from_path() - load an image using a file path
  *
@@ -1940,7 +2004,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
 {
 	efi_handle_t device;
 	efi_status_t ret;
-	struct efi_device_path *dp, *rem;
+	struct efi_device_path *rem;
 	struct efi_load_file_protocol *load_file_protocol = NULL;
 	efi_uintn_t buffer_size;
 	uint64_t addr, pages;
@@ -1950,12 +2014,15 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
 	*buffer = NULL;
 	*size = 0;
 
-	dp = file_path;
-	device = efi_dp_find_obj(dp, NULL, &rem);
-	ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
-				  NULL);
+	/* try first for simple file system protocols */
+	ret = efi_load_image_from_file(file_path, buffer, size);
 	if (ret == EFI_SUCCESS)
-		return efi_load_image_from_file(file_path, buffer, size);
+		return ret;
+
+	/* TODO: does this really make sense? */
+	device = efi_dp_find_obj(file_path, NULL, &rem);
+	if (!device)
+		return EFI_NOT_FOUND;
 
 	ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL);
 	if (ret == EFI_SUCCESS) {
diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index 7a7077e6d032..2d6a432b168b 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -1083,16 +1083,16 @@ static const struct efi_file_handle efi_file_handle_protocol = {
 /**
  * efi_file_from_path() - open file via device path
  *
- * @fp:		device path
+ * @v:		simple file system
+ * @fp:		file path
  * Return:	EFI_FILE_PROTOCOL for the file or NULL
  */
-struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
+struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,
+					 struct efi_device_path *fp)
 {
-	struct efi_simple_file_system_protocol *v;
 	struct efi_file_handle *f;
 	efi_status_t ret;
 
-	v = efi_fs_from_path(fp);
 	if (!v)
 		return NULL;
 
@@ -1100,10 +1100,6 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
 	if (ret != EFI_SUCCESS)
 		return NULL;
 
-	/* Skip over device-path nodes before the file path. */
-	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
-		fp = efi_dp_next(fp);
-
 	/*
 	 * Step through the nodes of the directory path until the actual file
 	 * node is reached which is the final node in the device path.
@@ -1138,6 +1134,29 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
 	return f;
 }
 
+/**
+ * efi_file_from_path() - open file via device path
+ *
+ * @fp:		device path
+ * Return:	EFI_FILE_PROTOCOL for the file or NULL
+ */
+struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
+{
+	struct efi_simple_file_system_protocol *v;
+
+	v = efi_fs_from_path(fp);
+	if (!v)
+		return NULL;
+
+	/* Skip over device-path nodes before the file path. */
+	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
+		fp = efi_dp_next(fp);
+	if (!fp)
+		return NULL;
+
+	return efi_file_from_fs(v, fp);
+}
+
 static efi_status_t EFIAPI
 efi_open_volume(struct efi_simple_file_system_protocol *this,
 		struct efi_file_handle **root)
-- 
2.33.0

Re: [PATCH] efi_loader: fix a problem in loading an image from a short-path

[Heinrich Schuchardt]

On 4/28/22 06:50, AKASHI Takahiro wrote:
> Booting from a short-form device path which starts with the first element
> being a File Path Media Device Path failed because it doesn't contain
> any valid device with simple file system protocol and efi_dp_find_obj()
> in efi_load_image_from_path() will return NULL.
> For instance,
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/\helloworld.efi
>    -> shortened version: /\helloworld.efi

Thanks for enabling this.

Please, enhance test/py/tests/test_efi_bootmgr to test booting from a
filename only device path.

>
> With this patch applied, all the media devices with simple file system
> protocol are enumerated and the boot manager attempts to boot temporarily
> generated device paths one-by-one.
>
> This new implementation is still a bit incompatible with the UEFI
> specification in terms of:
> * not creating real boot options

The UEFI specification has:

"These new boot options must not be saved to non volatile storage,
and may not be added to BootOrder."

Is there really something missing?

> * not distinguishing removable media and fix media

struct blk_desc has field 'removable' which is mirrored in struct
efi_disk_obj.

mmc_bind() always sets removable to 1 irrespective of the device being
eMMC or SD-card. I guess this would need to be fixed.

> (See section 3.1.3 "Boot Options".)
>
> But it still gives us a closer and better solution than the current.
>
> Fixes: commit 9cdf470274ff ("efi_loader: support booting via short-form device-path")
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   include/efi_loader.h          |  3 ++
>   lib/efi_loader/efi_boottime.c | 87 +++++++++++++++++++++++++++++++----
>   lib/efi_loader/efi_file.c     | 35 ++++++++++----
>   3 files changed, 107 insertions(+), 18 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index ba79a9afb404..9730c1375a55 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -661,6 +661,9 @@ void efi_signal_event(struct efi_event *event);
>   struct efi_simple_file_system_protocol *efi_simple_file_system(
>   		struct blk_desc *desc, int part, struct efi_device_path *dp);
>
> +/* open file from simple file system */
> +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,
> +					 struct efi_device_path *fp);
>   /* open file from device-path: */
>   struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp);
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 5bcb8253edba..39b0e8f7ade0 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1868,19 +1868,21 @@ out:
>   }
>
>   /**
> - * efi_load_image_from_file() - load an image from file system
> + * __efi_load_image_from_file() - load an image from file system
>    *
>    * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
>    * callers obligation to update the memory type as needed.
>    *
> + * @v:			simple file system

Please, use a meaningful variable name.

>    * @file_path:		the path of the image to load
>    * @buffer:		buffer containing the loaded image
>    * @size:		size of the loaded image
>    * Return:		status code
>    */
>   static
> -efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> -				      void **buffer, efi_uintn_t *size)
> +efi_status_t __efi_load_image_from_file(struct efi_simple_file_system_protocol *v,
> +					struct efi_device_path *file_path,
> +					void **buffer, efi_uintn_t *size)
>   {
>   	struct efi_file_handle *f;
>   	efi_status_t ret;
> @@ -1888,7 +1890,11 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
>   	efi_uintn_t bs;
>
>   	/* Open file */
> -	f = efi_file_from_path(file_path);
> +	if (v)
> +		f = efi_file_from_fs(v, file_path);
> +	else
> +		/* file_path should have a device path */
> +		f = efi_file_from_path(file_path);
>   	if (!f)
>   		return EFI_NOT_FOUND;
>
> @@ -1921,6 +1927,64 @@ error:
>   	return ret;
>   }
>
> +/**
> + * efi_load_image_from_file() - load an image from file system
> + *
> + * 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
> + */
> +static
> +efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> +				      void **buffer, efi_uintn_t *size)
> +{
> +	efi_uintn_t no_handles;
> +	efi_handle_t *handles;
> +	struct efi_handler *handler;
> +	struct efi_simple_file_system_protocol *fs;
> +	int i;
> +	efi_status_t ret;
> +
> +	/* if a file_path contains a device path */
> +	if (!EFI_DP_TYPE(file_path, MEDIA_DEVICE, FILE_PATH))
> +		return __efi_load_image_from_file(NULL, file_path, buffer, size);
> +
> +	/* no explicit device specified */
> +	ret = EFI_CALL(efi_locate_handle_buffer(
> +					BY_PROTOCOL,
> +					&efi_simple_file_system_protocol_guid,
> +					NULL,
> +					&no_handles,
> +					&handles));
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	if (!no_handles)
> +		return EFI_NOT_FOUND;
> +
> +	for (i = 0; i < no_handles; i++) {
> +		ret = efi_search_protocol(handles[i],
> +					  &efi_simple_file_system_protocol_guid,
> +					  &handler);
> +		if (ret != EFI_SUCCESS)
> +			/* unlikely */
> +			continue;
> +
> +		fs = handler->protocol_interface;
> +		if (!fs)
> +			continue;
> +
> +		ret = __efi_load_image_from_file(fs, file_path, buffer, size);
> +		if (ret == EFI_SUCCESS)
> +			return ret;
> +	}
> +
> +	return EFI_NOT_FOUND;
> +}
> +
>   /**
>    * efi_load_image_from_path() - load an image using a file path
>    *
> @@ -1940,7 +2004,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
>   {
>   	efi_handle_t device;
>   	efi_status_t ret;
> -	struct efi_device_path *dp, *rem;
> +	struct efi_device_path *rem;
>   	struct efi_load_file_protocol *load_file_protocol = NULL;
>   	efi_uintn_t buffer_size;
>   	uint64_t addr, pages;
> @@ -1950,12 +2014,15 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
>   	*buffer = NULL;
>   	*size = 0;
>
> -	dp = file_path;
> -	device = efi_dp_find_obj(dp, NULL, &rem);
> -	ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
> -				  NULL);
> +	/* try first for simple file system protocols */
> +	ret = efi_load_image_from_file(file_path, buffer, size);
>   	if (ret == EFI_SUCCESS)
> -		return efi_load_image_from_file(file_path, buffer, size);
> +		return ret;
> +
> +	/* TODO: does this really make sense? */
> +	device = efi_dp_find_obj(file_path, NULL, &rem);
> +	if (!device)
> +		return EFI_NOT_FOUND;
>
>   	ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL);
>   	if (ret == EFI_SUCCESS) {
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index 7a7077e6d032..2d6a432b168b 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -1083,16 +1083,16 @@ static const struct efi_file_handle efi_file_handle_protocol = {
>   /**
>    * efi_file_from_path() - open file via device path
>    *
> - * @fp:		device path
> + * @v:		simple file system
> + * @fp:		file path
>    * Return:	EFI_FILE_PROTOCOL for the file or NULL
>    */
> -struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,
> +					 struct efi_device_path *fp)
>   {
> -	struct efi_simple_file_system_protocol *v;
>   	struct efi_file_handle *f;
>   	efi_status_t ret;
>
> -	v = efi_fs_from_path(fp);
>   	if (!v)
>   		return NULL;
>
> @@ -1100,10 +1100,6 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
>   	if (ret != EFI_SUCCESS)
>   		return NULL;
>
> -	/* Skip over device-path nodes before the file path. */
> -	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> -		fp = efi_dp_next(fp);
> -
>   	/*
>   	 * Step through the nodes of the directory path until the actual file
>   	 * node is reached which is the final node in the device path.
> @@ -1138,6 +1134,29 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
>   	return f;
>   }
>
> +/**
> + * efi_file_from_path() - open file via device path
> + *
> + * @fp:		device path

'fp' does not resonate 'device path'. How about using dp?

> + * Return:	EFI_FILE_PROTOCOL for the file or NULL
> + */
> +struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> +{
> +	struct efi_simple_file_system_protocol *v;

Please, provide a meaningful variable name.

Best regards

Heinrich

> +
> +	v = efi_fs_from_path(fp);
> +	if (!v)
> +		return NULL;
> +
> +	/* Skip over device-path nodes before the file path. */
> +	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> +		fp = efi_dp_next(fp);
> +	if (!fp)
> +		return NULL;
> +
> +	return efi_file_from_fs(v, fp);
> +}
> +
>   static efi_status_t EFIAPI
>   efi_open_volume(struct efi_simple_file_system_protocol *this,
>   		struct efi_file_handle **root)

Re: [PATCH] efi_loader: fix a problem in loading an image from a short-path

[AKASHI Takahiro]

On Fri, Apr 29, 2022 at 12:57:15PM +0200, Heinrich Schuchardt wrote:
> On 4/28/22 06:50, AKASHI Takahiro wrote:
> > Booting from a short-form device path which starts with the first element
> > being a File Path Media Device Path failed because it doesn't contain
> > any valid device with simple file system protocol and efi_dp_find_obj()
> > in efi_load_image_from_path() will return NULL.
> > For instance,
> > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/\helloworld.efi
> >    -> shortened version: /\helloworld.efi
> 
> Thanks for enabling this.
> 
> Please, enhance test/py/tests/test_efi_bootmgr to test booting from a
> filename only device path.

Okay, but you should have added tests in your patch.

> > 
> > With this patch applied, all the media devices with simple file system
> > protocol are enumerated and the boot manager attempts to boot temporarily
> > generated device paths one-by-one.
> > 
> > This new implementation is still a bit incompatible with the UEFI
> > specification in terms of:
> > * not creating real boot options
> 
> The UEFI specification has:
> 
> "These new boot options must not be saved to non volatile storage,
> and may not be added to BootOrder."
> 
> Is there really something missing?

        When the boot manager attempts to boot a short-form File Path
        Media Device Path, it will enumerate all removable media devices, followed
        by all fixed media devices, creating boot options for each device.
                                    ^^^^^^^^^^^^^^^^^^^^^

> > * not distinguishing removable media and fix media
> 
> struct blk_desc has field 'removable' which is mirrored in struct
> efi_disk_obj.

Instead of relying on such an internal structure, we can and should
use a public interface, efi_block_io(->media.removable_media).

> mmc_bind() always sets removable to 1 irrespective of the device being
> eMMC or SD-card. I guess this would need to be fixed.
> 
> > (See section 3.1.3 "Boot Options".)
> > 
> > But it still gives us a closer and better solution than the current.
> > 
> > Fixes: commit 9cdf470274ff ("efi_loader: support booting via short-form device-path")
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   include/efi_loader.h          |  3 ++
> >   lib/efi_loader/efi_boottime.c | 87 +++++++++++++++++++++++++++++++----
> >   lib/efi_loader/efi_file.c     | 35 ++++++++++----
> >   3 files changed, 107 insertions(+), 18 deletions(-)
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index ba79a9afb404..9730c1375a55 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -661,6 +661,9 @@ void efi_signal_event(struct efi_event *event);
> >   struct efi_simple_file_system_protocol *efi_simple_file_system(
> >   		struct blk_desc *desc, int part, struct efi_device_path *dp);
> > 
> > +/* open file from simple file system */
> > +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,
> > +					 struct efi_device_path *fp);
> >   /* open file from device-path: */
> >   struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp);
> > 
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 5bcb8253edba..39b0e8f7ade0 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1868,19 +1868,21 @@ out:
> >   }
> > 
> >   /**
> > - * efi_load_image_from_file() - load an image from file system
> > + * __efi_load_image_from_file() - load an image from file system
> >    *
> >    * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
> >    * callers obligation to update the memory type as needed.
> >    *
> > + * @v:			simple file system
> 
> Please, use a meaningful variable name.

I will fully re-write this patch.
While efi boot manager should be responsible for handling a short-form device path
starting with a file path, my current implementation is made in
efi_load_image_from_path() hence LoadImage API.

Instead, the logic of enumerating file_system_protocol devices will be added
to try_load_entry()/efi_bootmgr_load().

-Takahiro Akashi


> >    * @file_path:		the path of the image to load
> >    * @buffer:		buffer containing the loaded image
> >    * @size:		size of the loaded image
> >    * Return:		status code
> >    */
> >   static
> > -efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> > -				      void **buffer, efi_uintn_t *size)
> > +efi_status_t __efi_load_image_from_file(struct efi_simple_file_system_protocol *v,
> > +					struct efi_device_path *file_path,
> > +					void **buffer, efi_uintn_t *size)
> >   {
> >   	struct efi_file_handle *f;
> >   	efi_status_t ret;
> > @@ -1888,7 +1890,11 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> >   	efi_uintn_t bs;
> > 
> >   	/* Open file */
> > -	f = efi_file_from_path(file_path);
> > +	if (v)
> > +		f = efi_file_from_fs(v, file_path);
> > +	else
> > +		/* file_path should have a device path */
> > +		f = efi_file_from_path(file_path);
> >   	if (!f)
> >   		return EFI_NOT_FOUND;
> > 
> > @@ -1921,6 +1927,64 @@ error:
> >   	return ret;
> >   }
> > 
> > +/**
> > + * efi_load_image_from_file() - load an image from file system
> > + *
> > + * 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
> > + */
> > +static
> > +efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> > +				      void **buffer, efi_uintn_t *size)
> > +{
> > +	efi_uintn_t no_handles;
> > +	efi_handle_t *handles;
> > +	struct efi_handler *handler;
> > +	struct efi_simple_file_system_protocol *fs;
> > +	int i;
> > +	efi_status_t ret;
> > +
> > +	/* if a file_path contains a device path */
> > +	if (!EFI_DP_TYPE(file_path, MEDIA_DEVICE, FILE_PATH))
> > +		return __efi_load_image_from_file(NULL, file_path, buffer, size);
> > +
> > +	/* no explicit device specified */
> > +	ret = EFI_CALL(efi_locate_handle_buffer(
> > +					BY_PROTOCOL,
> > +					&efi_simple_file_system_protocol_guid,
> > +					NULL,
> > +					&no_handles,
> > +					&handles));
> > +	if (ret != EFI_SUCCESS)
> > +		return ret;
> > +	if (!no_handles)
> > +		return EFI_NOT_FOUND;
> > +
> > +	for (i = 0; i < no_handles; i++) {
> > +		ret = efi_search_protocol(handles[i],
> > +					  &efi_simple_file_system_protocol_guid,
> > +					  &handler);
> > +		if (ret != EFI_SUCCESS)
> > +			/* unlikely */
> > +			continue;
> > +
> > +		fs = handler->protocol_interface;
> > +		if (!fs)
> > +			continue;
> > +
> > +		ret = __efi_load_image_from_file(fs, file_path, buffer, size);
> > +		if (ret == EFI_SUCCESS)
> > +			return ret;
> > +	}
> > +
> > +	return EFI_NOT_FOUND;
> > +}
> > +
> >   /**
> >    * efi_load_image_from_path() - load an image using a file path
> >    *
> > @@ -1940,7 +2004,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
> >   {
> >   	efi_handle_t device;
> >   	efi_status_t ret;
> > -	struct efi_device_path *dp, *rem;
> > +	struct efi_device_path *rem;
> >   	struct efi_load_file_protocol *load_file_protocol = NULL;
> >   	efi_uintn_t buffer_size;
> >   	uint64_t addr, pages;
> > @@ -1950,12 +2014,15 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
> >   	*buffer = NULL;
> >   	*size = 0;
> > 
> > -	dp = file_path;
> > -	device = efi_dp_find_obj(dp, NULL, &rem);
> > -	ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
> > -				  NULL);
> > +	/* try first for simple file system protocols */
> > +	ret = efi_load_image_from_file(file_path, buffer, size);
> >   	if (ret == EFI_SUCCESS)
> > -		return efi_load_image_from_file(file_path, buffer, size);
> > +		return ret;
> > +
> > +	/* TODO: does this really make sense? */
> > +	device = efi_dp_find_obj(file_path, NULL, &rem);
> > +	if (!device)
> > +		return EFI_NOT_FOUND;
> > 
> >   	ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL);
> >   	if (ret == EFI_SUCCESS) {
> > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> > index 7a7077e6d032..2d6a432b168b 100644
> > --- a/lib/efi_loader/efi_file.c
> > +++ b/lib/efi_loader/efi_file.c
> > @@ -1083,16 +1083,16 @@ static const struct efi_file_handle efi_file_handle_protocol = {
> >   /**
> >    * efi_file_from_path() - open file via device path
> >    *
> > - * @fp:		device path
> > + * @v:		simple file system
> > + * @fp:		file path
> >    * Return:	EFI_FILE_PROTOCOL for the file or NULL
> >    */
> > -struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> > +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,
> > +					 struct efi_device_path *fp)
> >   {
> > -	struct efi_simple_file_system_protocol *v;
> >   	struct efi_file_handle *f;
> >   	efi_status_t ret;
> > 
> > -	v = efi_fs_from_path(fp);
> >   	if (!v)
> >   		return NULL;
> > 
> > @@ -1100,10 +1100,6 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> >   	if (ret != EFI_SUCCESS)
> >   		return NULL;
> > 
> > -	/* Skip over device-path nodes before the file path. */
> > -	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> > -		fp = efi_dp_next(fp);
> > -
> >   	/*
> >   	 * Step through the nodes of the directory path until the actual file
> >   	 * node is reached which is the final node in the device path.
> > @@ -1138,6 +1134,29 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> >   	return f;
> >   }
> > 
> > +/**
> > + * efi_file_from_path() - open file via device path
> > + *
> > + * @fp:		device path
> 
> 'fp' does not resonate 'device path'. How about using dp?
> 
> > + * Return:	EFI_FILE_PROTOCOL for the file or NULL
> > + */
> > +struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> > +{
> > +	struct efi_simple_file_system_protocol *v;
> 
> Please, provide a meaningful variable name.
> 
> Best regards
> 
> Heinrich
> 
> > +
> > +	v = efi_fs_from_path(fp);
> > +	if (!v)
> > +		return NULL;
> > +
> > +	/* Skip over device-path nodes before the file path. */
> > +	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> > +		fp = efi_dp_next(fp);
> > +	if (!fp)
> > +		return NULL;
> > +
> > +	return efi_file_from_fs(v, fp);
> > +}
> > +
> >   static efi_status_t EFIAPI
> >   efi_open_volume(struct efi_simple_file_system_protocol *this,
> >   		struct efi_file_handle **root)
>