[PATCH 0/3] efi_loader: setting boot device

[PATCH 0/3] efi_loader: setting boot device

[Heinrich Schuchardt]

Up to now the bootefi command uses the last file loaded to determine the
boot partition. This has led to user irritation when the fdt has been
loaded from another partition after the EFI binary.

Before setting the boot device from a loaded file check if it is a PE-COFF
image.

The first patch carves out the test function.
The second make use of the PE-COFF check.
The third add the display of the boot device and file path for easier testing.

Heinrich Schuchardt (3):
  efi_loader: print boot device and file path in helloworld
  efi_loader: carve out efi_check_pe()
  efi_loader: setting boot device

 cmd/bootefi.c                     |  14 ++-
 doc/uefi/uefi.rst                 |  11 +-
 fs/fs.c                           |   3 +-
 include/efi_loader.h              |   8 +-
 lib/efi_loader/efi_image_loader.c |  80 ++++++++------
 lib/efi_loader/helloworld.c       | 167 ++++++++++++++++++++++++------
 net/tftp.c                        |   9 +-
 7 files changed, 211 insertions(+), 81 deletions(-)

--
2.29.2

[PATCH 1/3] efi_loader: print boot device and file path in helloworld

[Heinrich Schuchardt]

Let helloworld.efi print the device path of the boot device and the file
path as provided by the loaded image protocol.

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

diff --git a/lib/efi_loader/helloworld.c b/lib/efi_loader/helloworld.c
index 9ae2ee3389..5c8b7a96f9 100644
--- a/lib/efi_loader/helloworld.c
+++ b/lib/efi_loader/helloworld.c
@@ -1,43 +1,41 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * EFI hello world
+ * Hello world EFI application
  *
- * Copyright (c) 2016 Google, Inc
- * Written by Simon Glass <sjg@chromium.org>
+ * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
  *
- * This program demonstrates calling a boottime service.
- * It writes a greeting and the load options to the console.
+ * This test program is used to test the invocation of an EFI application.
+ * It writes
+ *
+ * * a greeting
+ * * the firmware's UEFI version
+ * * the installed configuration tables
+ * * the boot device's device path and the file path
+ *
+ * to the console.
  */

-#include <common.h>
 #include <efi_api.h>

 static const efi_guid_t loaded_image_guid = EFI_LOADED_IMAGE_PROTOCOL_GUID;
+static const efi_guid_t device_path_to_text_protocol_guid =
+	EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
+static const efi_guid_t device_path_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
 static const efi_guid_t fdt_guid = EFI_FDT_GUID;
 static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
 static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;

+static struct efi_system_table *systable;
+static struct efi_boot_services *boottime;
+static struct efi_simple_text_output_protocol *con_out;
+
 /**
- * efi_main() - entry point of the EFI application.
- *
- * @handle:	handle of the loaded image
- * @systable:	system table
- * @return:	status code
+ * print_uefi_revision() - print UEFI revision number
  */
-efi_status_t EFIAPI efi_main(efi_handle_t handle,
-			     struct efi_system_table *systable)
+static void print_uefi_revision(void)
 {
-	struct efi_simple_text_output_protocol *con_out = systable->con_out;
-	struct efi_boot_services *boottime = systable->boottime;
-	struct efi_loaded_image *loaded_image;
-	efi_status_t ret;
-	efi_uintn_t i;
 	u16 rev[] = L"0.0.0";

-	/* UEFI requires CR LF */
-	con_out->output_string(con_out, L"Hello, world!\r\n");
-
-	/* Print the revision number */
 	rev[0] = (systable->hdr.revision >> 16) + '0';
 	rev[4] = systable->hdr.revision & 0xffff;
 	for (; rev[4] >= 10;) {
@@ -53,15 +51,15 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
 	con_out->output_string(con_out, L"Running on UEFI ");
 	con_out->output_string(con_out, rev);
 	con_out->output_string(con_out, L"\r\n");
+}
+
+/**
+ * print_config_tables() - print configuration tables
+ */
+static void print_config_tables(void)
+{
+	efi_uintn_t i;

-	/* Get the loaded image protocol */
-	ret = boottime->handle_protocol(handle, &loaded_image_guid,
-					(void **)&loaded_image);
-	if (ret != EFI_SUCCESS) {
-		con_out->output_string
-			(con_out, L"Cannot open loaded image protocol\r\n");
-		goto out;
-	}
 	/* Find configuration tables */
 	for (i = 0; i < systable->nr_tables; ++i) {
 		if (!memcmp(&systable->tables[i].guid, &fdt_guid,
@@ -77,6 +75,16 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
 			con_out->output_string
 					(con_out, L"Have SMBIOS table\r\n");
 	}
+}
+
+/**
+ * print_load_options() - print load options
+ *
+ * @systable:	system table
+ * @con_out:	simple text output protocol
+ */
+void print_load_options(struct efi_loaded_image *loaded_image)
+{
 	/* Output the load options */
 	con_out->output_string(con_out, L"Load options: ");
 	if (loaded_image->load_options_size && loaded_image->load_options)
@@ -85,6 +93,105 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
 	else
 		con_out->output_string(con_out, L"<none>");
 	con_out->output_string(con_out, L"\r\n");
+}
+
+/**
+ * print_device_path() - print device path
+ *
+ * @device_path:	device path to print
+ * @dp2txt:		device path to text protocol
+ */
+efi_status_t print_device_path(struct efi_device_path *device_path,
+			       struct efi_device_path_to_text_protocol *dp2txt)
+{
+	u16 *string;
+	efi_status_t ret;
+
+	if (!device_path) {
+		con_out->output_string(con_out, L"<none>\r\n");
+		return EFI_SUCCESS;
+	}
+
+	string = dp2txt->convert_device_path_to_text(device_path, true, false);
+	if (!string) {
+		con_out->output_string
+			(con_out, L"Cannot convert device path to text\r\n");
+		return EFI_OUT_OF_RESOURCES;
+	}
+	con_out->output_string(con_out, string);
+	con_out->output_string(con_out, L"\r\n");
+	ret = boottime->free_pool(string);
+	if (ret != EFI_SUCCESS) {
+		con_out->output_string(con_out, L"Cannot free pool memory\r\n");
+		return ret;
+	}
+	return EFI_SUCCESS;
+}
+
+/**
+ * efi_main() - entry point of the EFI application.
+ *
+ * @handle:	handle of the loaded image
+ * @systab:	system table
+ * @return:	status code
+ */
+efi_status_t EFIAPI efi_main(efi_handle_t handle,
+			     struct efi_system_table *systab)
+{
+	struct efi_loaded_image *loaded_image;
+	struct efi_device_path_to_text_protocol *device_path_to_text;
+	struct efi_device_path *device_path;
+	efi_status_t ret;
+
+	systable = systab;
+	boottime = systable->boottime;
+	con_out = systable->con_out;
+
+	/* UEFI requires CR LF */
+	con_out->output_string(con_out, L"Hello, world!\r\n");
+
+	print_uefi_revision();
+	print_config_tables();
+
+	/* Get the loaded image protocol */
+	ret = boottime->handle_protocol(handle, &loaded_image_guid,
+					(void **)&loaded_image);
+	if (ret != EFI_SUCCESS) {
+		con_out->output_string
+			(con_out, L"Cannot open loaded image protocol\r\n");
+		goto out;
+	}
+	print_load_options(loaded_image);
+
+	/* Get the device path to text protocol */
+	ret = boottime->locate_protocol(&device_path_to_text_protocol_guid,
+					NULL, (void **)&device_path_to_text);
+	if (ret != EFI_SUCCESS) {
+		con_out->output_string
+			(con_out, L"Cannot open device path to text protocol\r\n");
+		goto out;
+	}
+	if (!loaded_image->device_handle) {
+		con_out->output_string
+			(con_out, L"Missing device handle\r\n");
+		goto out;
+	}
+	ret = boottime->handle_protocol(loaded_image->device_handle,
+					&device_path_guid,
+					(void **)&device_path);
+	if (ret != EFI_SUCCESS) {
+		con_out->output_string
+			(con_out, L"Missing devide path for device handle\r\n");
+		goto out;
+	}
+	con_out->output_string(con_out, L"Boot device: ");
+	ret = print_device_path(device_path, device_path_to_text);
+	if (ret != EFI_SUCCESS)
+		goto out;
+	con_out->output_string(con_out, L"File path: ");
+	ret = print_device_path(loaded_image->file_path, device_path_to_text);
+	if (ret != EFI_SUCCESS)
+		goto out;

 out:
 	boottime->exit(handle, ret, 0, NULL);
--
2.29.2

[PATCH 1/3] efi_loader: print boot device and file path in helloworld

[Simon Glass]

On Tue, 12 Jan 2021 at 12:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Let helloworld.efi print the device path of the boot device and the file
> path as provided by the loaded image protocol.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/helloworld.c | 167 +++++++++++++++++++++++++++++-------
>  1 file changed, 137 insertions(+), 30 deletions(-)

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

[PATCH 1/3] efi_loader: print boot device and file path in helloworld

[AKASHI Takahiro]

Heinrich,

On Tue, Jan 12, 2021 at 08:58:40PM +0100, Heinrich Schuchardt wrote:
> Let helloworld.efi print the device path of the boot device and the file
> path as provided by the loaded image protocol.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/helloworld.c | 167 +++++++++++++++++++++++++++++-------
>  1 file changed, 137 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/efi_loader/helloworld.c b/lib/efi_loader/helloworld.c
> index 9ae2ee3389..5c8b7a96f9 100644
> --- a/lib/efi_loader/helloworld.c
> +++ b/lib/efi_loader/helloworld.c
> @@ -1,43 +1,41 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * EFI hello world
> + * Hello world EFI application
>   *
> - * Copyright (c) 2016 Google, Inc
> - * Written by Simon Glass <sjg@chromium.org>
> + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
>   *
> - * This program demonstrates calling a boottime service.
> - * It writes a greeting and the load options to the console.
> + * This test program is used to test the invocation of an EFI application.
> + * It writes
> + *
> + * * a greeting
> + * * the firmware's UEFI version
> + * * the installed configuration tables
> + * * the boot device's device path and the file path

If this kind of information is quite useful for users, why not add
that (printing) feature as an option of bootefi (or efidebug)?
I'm afraid that most users who are irritated as you said won't be able
to imagine such information be printed by helloworld app.

-Takahiro Akashi

> + *
> + * to the console.
>   */
> 
> -#include <common.h>
>  #include <efi_api.h>
> 
>  static const efi_guid_t loaded_image_guid = EFI_LOADED_IMAGE_PROTOCOL_GUID;
> +static const efi_guid_t device_path_to_text_protocol_guid =
> +	EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
> +static const efi_guid_t device_path_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
>  static const efi_guid_t fdt_guid = EFI_FDT_GUID;
>  static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
>  static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
> 
> +static struct efi_system_table *systable;
> +static struct efi_boot_services *boottime;
> +static struct efi_simple_text_output_protocol *con_out;
> +
>  /**
> - * efi_main() - entry point of the EFI application.
> - *
> - * @handle:	handle of the loaded image
> - * @systable:	system table
> - * @return:	status code
> + * print_uefi_revision() - print UEFI revision number
>   */
> -efi_status_t EFIAPI efi_main(efi_handle_t handle,
> -			     struct efi_system_table *systable)
> +static void print_uefi_revision(void)
>  {
> -	struct efi_simple_text_output_protocol *con_out = systable->con_out;
> -	struct efi_boot_services *boottime = systable->boottime;
> -	struct efi_loaded_image *loaded_image;
> -	efi_status_t ret;
> -	efi_uintn_t i;
>  	u16 rev[] = L"0.0.0";
> 
> -	/* UEFI requires CR LF */
> -	con_out->output_string(con_out, L"Hello, world!\r\n");
> -
> -	/* Print the revision number */
>  	rev[0] = (systable->hdr.revision >> 16) + '0';
>  	rev[4] = systable->hdr.revision & 0xffff;
>  	for (; rev[4] >= 10;) {
> @@ -53,15 +51,15 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
>  	con_out->output_string(con_out, L"Running on UEFI ");
>  	con_out->output_string(con_out, rev);
>  	con_out->output_string(con_out, L"\r\n");
> +}
> +
> +/**
> + * print_config_tables() - print configuration tables
> + */
> +static void print_config_tables(void)
> +{
> +	efi_uintn_t i;
> 
> -	/* Get the loaded image protocol */
> -	ret = boottime->handle_protocol(handle, &loaded_image_guid,
> -					(void **)&loaded_image);
> -	if (ret != EFI_SUCCESS) {
> -		con_out->output_string
> -			(con_out, L"Cannot open loaded image protocol\r\n");
> -		goto out;
> -	}
>  	/* Find configuration tables */
>  	for (i = 0; i < systable->nr_tables; ++i) {
>  		if (!memcmp(&systable->tables[i].guid, &fdt_guid,
> @@ -77,6 +75,16 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
>  			con_out->output_string
>  					(con_out, L"Have SMBIOS table\r\n");
>  	}
> +}
> +
> +/**
> + * print_load_options() - print load options
> + *
> + * @systable:	system table
> + * @con_out:	simple text output protocol
> + */
> +void print_load_options(struct efi_loaded_image *loaded_image)
> +{
>  	/* Output the load options */
>  	con_out->output_string(con_out, L"Load options: ");
>  	if (loaded_image->load_options_size && loaded_image->load_options)
> @@ -85,6 +93,105 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
>  	else
>  		con_out->output_string(con_out, L"<none>");
>  	con_out->output_string(con_out, L"\r\n");
> +}
> +
> +/**
> + * print_device_path() - print device path
> + *
> + * @device_path:	device path to print
> + * @dp2txt:		device path to text protocol
> + */
> +efi_status_t print_device_path(struct efi_device_path *device_path,
> +			       struct efi_device_path_to_text_protocol *dp2txt)
> +{
> +	u16 *string;
> +	efi_status_t ret;
> +
> +	if (!device_path) {
> +		con_out->output_string(con_out, L"<none>\r\n");
> +		return EFI_SUCCESS;
> +	}
> +
> +	string = dp2txt->convert_device_path_to_text(device_path, true, false);
> +	if (!string) {
> +		con_out->output_string
> +			(con_out, L"Cannot convert device path to text\r\n");
> +		return EFI_OUT_OF_RESOURCES;
> +	}
> +	con_out->output_string(con_out, string);
> +	con_out->output_string(con_out, L"\r\n");
> +	ret = boottime->free_pool(string);
> +	if (ret != EFI_SUCCESS) {
> +		con_out->output_string(con_out, L"Cannot free pool memory\r\n");
> +		return ret;
> +	}
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_main() - entry point of the EFI application.
> + *
> + * @handle:	handle of the loaded image
> + * @systab:	system table
> + * @return:	status code
> + */
> +efi_status_t EFIAPI efi_main(efi_handle_t handle,
> +			     struct efi_system_table *systab)
> +{
> +	struct efi_loaded_image *loaded_image;
> +	struct efi_device_path_to_text_protocol *device_path_to_text;
> +	struct efi_device_path *device_path;
> +	efi_status_t ret;
> +
> +	systable = systab;
> +	boottime = systable->boottime;
> +	con_out = systable->con_out;
> +
> +	/* UEFI requires CR LF */
> +	con_out->output_string(con_out, L"Hello, world!\r\n");
> +
> +	print_uefi_revision();
> +	print_config_tables();
> +
> +	/* Get the loaded image protocol */
> +	ret = boottime->handle_protocol(handle, &loaded_image_guid,
> +					(void **)&loaded_image);
> +	if (ret != EFI_SUCCESS) {
> +		con_out->output_string
> +			(con_out, L"Cannot open loaded image protocol\r\n");
> +		goto out;
> +	}
> +	print_load_options(loaded_image);
> +
> +	/* Get the device path to text protocol */
> +	ret = boottime->locate_protocol(&device_path_to_text_protocol_guid,
> +					NULL, (void **)&device_path_to_text);
> +	if (ret != EFI_SUCCESS) {
> +		con_out->output_string
> +			(con_out, L"Cannot open device path to text protocol\r\n");
> +		goto out;
> +	}
> +	if (!loaded_image->device_handle) {
> +		con_out->output_string
> +			(con_out, L"Missing device handle\r\n");
> +		goto out;
> +	}
> +	ret = boottime->handle_protocol(loaded_image->device_handle,
> +					&device_path_guid,
> +					(void **)&device_path);
> +	if (ret != EFI_SUCCESS) {
> +		con_out->output_string
> +			(con_out, L"Missing devide path for device handle\r\n");
> +		goto out;
> +	}
> +	con_out->output_string(con_out, L"Boot device: ");
> +	ret = print_device_path(device_path, device_path_to_text);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +	con_out->output_string(con_out, L"File path: ");
> +	ret = print_device_path(loaded_image->file_path, device_path_to_text);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> 
>  out:
>  	boottime->exit(handle, ret, 0, NULL);
> --
> 2.29.2
> 

[PATCH 1/3] efi_loader: print boot device and file path in helloworld

[Heinrich Schuchardt]

Am 15. Januar 2021 02:56:03 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>Heinrich,
>
>On Tue, Jan 12, 2021 at 08:58:40PM +0100, Heinrich Schuchardt wrote:
>> Let helloworld.efi print the device path of the boot device and the
>file
>> path as provided by the loaded image protocol.
>> 
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_loader/helloworld.c | 167
>+++++++++++++++++++++++++++++-------
>>  1 file changed, 137 insertions(+), 30 deletions(-)
>> 
>> diff --git a/lib/efi_loader/helloworld.c
>b/lib/efi_loader/helloworld.c
>> index 9ae2ee3389..5c8b7a96f9 100644
>> --- a/lib/efi_loader/helloworld.c
>> +++ b/lib/efi_loader/helloworld.c
>> @@ -1,43 +1,41 @@
>>  // SPDX-License-Identifier: GPL-2.0+
>>  /*
>> - * EFI hello world
>> + * Hello world EFI application
>>   *
>> - * Copyright (c) 2016 Google, Inc
>> - * Written by Simon Glass <sjg@chromium.org>
>> + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>   *
>> - * This program demonstrates calling a boottime service.
>> - * It writes a greeting and the load options to the console.
>> + * This test program is used to test the invocation of an EFI
>application.
>> + * It writes
>> + *
>> + * * a greeting
>> + * * the firmware's UEFI version
>> + * * the installed configuration tables
>> + * * the boot device's device path and the file path
>
>If this kind of information is quite useful for users, why not add
>that (printing) feature as an option of bootefi (or efidebug)?
>I'm afraid that most users who are irritated as you said won't be able
>to imagine such information be printed by helloworld app.
>

The file path is written in

https://github.com/trini/u-boot/blob/master/cmd/bootefi.c#L471

Device paths are not really user friendly. So I would not like to write it there.

Best regards

Heinrich


>-Takahiro Akashi
>
>> + *
>> + * to the console.
>>   */
>> 
>> -#include <common.h>
>>  #include <efi_api.h>
>> 
>>  static const efi_guid_t loaded_image_guid =
>EFI_LOADED_IMAGE_PROTOCOL_GUID;
>> +static const efi_guid_t device_path_to_text_protocol_guid =
>> +	EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
>> +static const efi_guid_t device_path_guid =
>EFI_DEVICE_PATH_PROTOCOL_GUID;
>>  static const efi_guid_t fdt_guid = EFI_FDT_GUID;
>>  static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
>>  static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
>> 
>> +static struct efi_system_table *systable;
>> +static struct efi_boot_services *boottime;
>> +static struct efi_simple_text_output_protocol *con_out;
>> +
>>  /**
>> - * efi_main() - entry point of the EFI application.
>> - *
>> - * @handle:	handle of the loaded image
>> - * @systable:	system table
>> - * @return:	status code
>> + * print_uefi_revision() - print UEFI revision number
>>   */
>> -efi_status_t EFIAPI efi_main(efi_handle_t handle,
>> -			     struct efi_system_table *systable)
>> +static void print_uefi_revision(void)
>>  {
>> -	struct efi_simple_text_output_protocol *con_out =
>systable->con_out;
>> -	struct efi_boot_services *boottime = systable->boottime;
>> -	struct efi_loaded_image *loaded_image;
>> -	efi_status_t ret;
>> -	efi_uintn_t i;
>>  	u16 rev[] = L"0.0.0";
>> 
>> -	/* UEFI requires CR LF */
>> -	con_out->output_string(con_out, L"Hello, world!\r\n");
>> -
>> -	/* Print the revision number */
>>  	rev[0] = (systable->hdr.revision >> 16) + '0';
>>  	rev[4] = systable->hdr.revision & 0xffff;
>>  	for (; rev[4] >= 10;) {
>> @@ -53,15 +51,15 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
>>  	con_out->output_string(con_out, L"Running on UEFI ");
>>  	con_out->output_string(con_out, rev);
>>  	con_out->output_string(con_out, L"\r\n");
>> +}
>> +
>> +/**
>> + * print_config_tables() - print configuration tables
>> + */
>> +static void print_config_tables(void)
>> +{
>> +	efi_uintn_t i;
>> 
>> -	/* Get the loaded image protocol */
>> -	ret = boottime->handle_protocol(handle, &loaded_image_guid,
>> -					(void **)&loaded_image);
>> -	if (ret != EFI_SUCCESS) {
>> -		con_out->output_string
>> -			(con_out, L"Cannot open loaded image protocol\r\n");
>> -		goto out;
>> -	}
>>  	/* Find configuration tables */
>>  	for (i = 0; i < systable->nr_tables; ++i) {
>>  		if (!memcmp(&systable->tables[i].guid, &fdt_guid,
>> @@ -77,6 +75,16 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
>>  			con_out->output_string
>>  					(con_out, L"Have SMBIOS table\r\n");
>>  	}
>> +}
>> +
>> +/**
>> + * print_load_options() - print load options
>> + *
>> + * @systable:	system table
>> + * @con_out:	simple text output protocol
>> + */
>> +void print_load_options(struct efi_loaded_image *loaded_image)
>> +{
>>  	/* Output the load options */
>>  	con_out->output_string(con_out, L"Load options: ");
>>  	if (loaded_image->load_options_size && loaded_image->load_options)
>> @@ -85,6 +93,105 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
>>  	else
>>  		con_out->output_string(con_out, L"<none>");
>>  	con_out->output_string(con_out, L"\r\n");
>> +}
>> +
>> +/**
>> + * print_device_path() - print device path
>> + *
>> + * @device_path:	device path to print
>> + * @dp2txt:		device path to text protocol
>> + */
>> +efi_status_t print_device_path(struct efi_device_path *device_path,
>> +			       struct efi_device_path_to_text_protocol *dp2txt)
>> +{
>> +	u16 *string;
>> +	efi_status_t ret;
>> +
>> +	if (!device_path) {
>> +		con_out->output_string(con_out, L"<none>\r\n");
>> +		return EFI_SUCCESS;
>> +	}
>> +
>> +	string = dp2txt->convert_device_path_to_text(device_path, true,
>false);
>> +	if (!string) {
>> +		con_out->output_string
>> +			(con_out, L"Cannot convert device path to text\r\n");
>> +		return EFI_OUT_OF_RESOURCES;
>> +	}
>> +	con_out->output_string(con_out, string);
>> +	con_out->output_string(con_out, L"\r\n");
>> +	ret = boottime->free_pool(string);
>> +	if (ret != EFI_SUCCESS) {
>> +		con_out->output_string(con_out, L"Cannot free pool memory\r\n");
>> +		return ret;
>> +	}
>> +	return EFI_SUCCESS;
>> +}
>> +
>> +/**
>> + * efi_main() - entry point of the EFI application.
>> + *
>> + * @handle:	handle of the loaded image
>> + * @systab:	system table
>> + * @return:	status code
>> + */
>> +efi_status_t EFIAPI efi_main(efi_handle_t handle,
>> +			     struct efi_system_table *systab)
>> +{
>> +	struct efi_loaded_image *loaded_image;
>> +	struct efi_device_path_to_text_protocol *device_path_to_text;
>> +	struct efi_device_path *device_path;
>> +	efi_status_t ret;
>> +
>> +	systable = systab;
>> +	boottime = systable->boottime;
>> +	con_out = systable->con_out;
>> +
>> +	/* UEFI requires CR LF */
>> +	con_out->output_string(con_out, L"Hello, world!\r\n");
>> +
>> +	print_uefi_revision();
>> +	print_config_tables();
>> +
>> +	/* Get the loaded image protocol */
>> +	ret = boottime->handle_protocol(handle, &loaded_image_guid,
>> +					(void **)&loaded_image);
>> +	if (ret != EFI_SUCCESS) {
>> +		con_out->output_string
>> +			(con_out, L"Cannot open loaded image protocol\r\n");
>> +		goto out;
>> +	}
>> +	print_load_options(loaded_image);
>> +
>> +	/* Get the device path to text protocol */
>> +	ret = boottime->locate_protocol(&device_path_to_text_protocol_guid,
>> +					NULL, (void **)&device_path_to_text);
>> +	if (ret != EFI_SUCCESS) {
>> +		con_out->output_string
>> +			(con_out, L"Cannot open device path to text protocol\r\n");
>> +		goto out;
>> +	}
>> +	if (!loaded_image->device_handle) {
>> +		con_out->output_string
>> +			(con_out, L"Missing device handle\r\n");
>> +		goto out;
>> +	}
>> +	ret = boottime->handle_protocol(loaded_image->device_handle,
>> +					&device_path_guid,
>> +					(void **)&device_path);
>> +	if (ret != EFI_SUCCESS) {
>> +		con_out->output_string
>> +			(con_out, L"Missing devide path for device handle\r\n");
>> +		goto out;
>> +	}
>> +	con_out->output_string(con_out, L"Boot device: ");
>> +	ret = print_device_path(device_path, device_path_to_text);
>> +	if (ret != EFI_SUCCESS)
>> +		goto out;
>> +	con_out->output_string(con_out, L"File path: ");
>> +	ret = print_device_path(loaded_image->file_path,
>device_path_to_text);
>> +	if (ret != EFI_SUCCESS)
>> +		goto out;
>> 
>>  out:
>>  	boottime->exit(handle, ret, 0, NULL);
>> --
>> 2.29.2
>> 

[PATCH 1/3] efi_loader: print boot device and file path in helloworld

[AKASHI Takahiro]

On Fri, Jan 15, 2021 at 04:12:18AM +0100, Heinrich Schuchardt wrote:
> Am 15. Januar 2021 02:56:03 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >Heinrich,
> >
> >On Tue, Jan 12, 2021 at 08:58:40PM +0100, Heinrich Schuchardt wrote:
> >> Let helloworld.efi print the device path of the boot device and the
> >file
> >> path as provided by the loaded image protocol.
> >> 
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  lib/efi_loader/helloworld.c | 167
> >+++++++++++++++++++++++++++++-------
> >>  1 file changed, 137 insertions(+), 30 deletions(-)
> >> 
> >> diff --git a/lib/efi_loader/helloworld.c
> >b/lib/efi_loader/helloworld.c
> >> index 9ae2ee3389..5c8b7a96f9 100644
> >> --- a/lib/efi_loader/helloworld.c
> >> +++ b/lib/efi_loader/helloworld.c
> >> @@ -1,43 +1,41 @@
> >>  // SPDX-License-Identifier: GPL-2.0+
> >>  /*
> >> - * EFI hello world
> >> + * Hello world EFI application
> >>   *
> >> - * Copyright (c) 2016 Google, Inc
> >> - * Written by Simon Glass <sjg@chromium.org>
> >> + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>   *
> >> - * This program demonstrates calling a boottime service.
> >> - * It writes a greeting and the load options to the console.
> >> + * This test program is used to test the invocation of an EFI
> >application.
> >> + * It writes
> >> + *
> >> + * * a greeting
> >> + * * the firmware's UEFI version
> >> + * * the installed configuration tables
> >> + * * the boot device's device path and the file path
> >
> >If this kind of information is quite useful for users, why not add
> >that (printing) feature as an option of bootefi (or efidebug)?
> >I'm afraid that most users who are irritated as you said won't be able
> >to imagine such information be printed by helloworld app.
> >
> 
> The file path is written in
> 
> https://github.com/trini/u-boot/blob/master/cmd/bootefi.c#L471
> 
> Device paths are not really user friendly.

So why do you want to print such info at helloworld?

I guess that, according to your cover letter, you have in your mind
some cases where an user may get in trouble relating to the boot device.
Right?

> So I would not like to write it there.

What I meant to suggest is to add an option, -v or -h, to bootefi,
which prints verbose (and helpful) information for users to identify a cause.
I can easily imagine users may blindly try to add -[v|h] when
they see an error message even if they don't know there is such an option:)

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> 
> >-Takahiro Akashi
> >
> >> + *
> >> + * to the console.
> >>   */
> >> 
> >> -#include <common.h>
> >>  #include <efi_api.h>
> >> 
> >>  static const efi_guid_t loaded_image_guid =
> >EFI_LOADED_IMAGE_PROTOCOL_GUID;
> >> +static const efi_guid_t device_path_to_text_protocol_guid =
> >> +	EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
> >> +static const efi_guid_t device_path_guid =
> >EFI_DEVICE_PATH_PROTOCOL_GUID;
> >>  static const efi_guid_t fdt_guid = EFI_FDT_GUID;
> >>  static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
> >>  static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
> >> 
> >> +static struct efi_system_table *systable;
> >> +static struct efi_boot_services *boottime;
> >> +static struct efi_simple_text_output_protocol *con_out;
> >> +
> >>  /**
> >> - * efi_main() - entry point of the EFI application.
> >> - *
> >> - * @handle:	handle of the loaded image
> >> - * @systable:	system table
> >> - * @return:	status code
> >> + * print_uefi_revision() - print UEFI revision number
> >>   */
> >> -efi_status_t EFIAPI efi_main(efi_handle_t handle,
> >> -			     struct efi_system_table *systable)
> >> +static void print_uefi_revision(void)
> >>  {
> >> -	struct efi_simple_text_output_protocol *con_out =
> >systable->con_out;
> >> -	struct efi_boot_services *boottime = systable->boottime;
> >> -	struct efi_loaded_image *loaded_image;
> >> -	efi_status_t ret;
> >> -	efi_uintn_t i;
> >>  	u16 rev[] = L"0.0.0";
> >> 
> >> -	/* UEFI requires CR LF */
> >> -	con_out->output_string(con_out, L"Hello, world!\r\n");
> >> -
> >> -	/* Print the revision number */
> >>  	rev[0] = (systable->hdr.revision >> 16) + '0';
> >>  	rev[4] = systable->hdr.revision & 0xffff;
> >>  	for (; rev[4] >= 10;) {
> >> @@ -53,15 +51,15 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
> >>  	con_out->output_string(con_out, L"Running on UEFI ");
> >>  	con_out->output_string(con_out, rev);
> >>  	con_out->output_string(con_out, L"\r\n");
> >> +}
> >> +
> >> +/**
> >> + * print_config_tables() - print configuration tables
> >> + */
> >> +static void print_config_tables(void)
> >> +{
> >> +	efi_uintn_t i;
> >> 
> >> -	/* Get the loaded image protocol */
> >> -	ret = boottime->handle_protocol(handle, &loaded_image_guid,
> >> -					(void **)&loaded_image);
> >> -	if (ret != EFI_SUCCESS) {
> >> -		con_out->output_string
> >> -			(con_out, L"Cannot open loaded image protocol\r\n");
> >> -		goto out;
> >> -	}
> >>  	/* Find configuration tables */
> >>  	for (i = 0; i < systable->nr_tables; ++i) {
> >>  		if (!memcmp(&systable->tables[i].guid, &fdt_guid,
> >> @@ -77,6 +75,16 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
> >>  			con_out->output_string
> >>  					(con_out, L"Have SMBIOS table\r\n");
> >>  	}
> >> +}
> >> +
> >> +/**
> >> + * print_load_options() - print load options
> >> + *
> >> + * @systable:	system table
> >> + * @con_out:	simple text output protocol
> >> + */
> >> +void print_load_options(struct efi_loaded_image *loaded_image)
> >> +{
> >>  	/* Output the load options */
> >>  	con_out->output_string(con_out, L"Load options: ");
> >>  	if (loaded_image->load_options_size && loaded_image->load_options)
> >> @@ -85,6 +93,105 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
> >>  	else
> >>  		con_out->output_string(con_out, L"<none>");
> >>  	con_out->output_string(con_out, L"\r\n");
> >> +}
> >> +
> >> +/**
> >> + * print_device_path() - print device path
> >> + *
> >> + * @device_path:	device path to print
> >> + * @dp2txt:		device path to text protocol
> >> + */
> >> +efi_status_t print_device_path(struct efi_device_path *device_path,
> >> +			       struct efi_device_path_to_text_protocol *dp2txt)
> >> +{
> >> +	u16 *string;
> >> +	efi_status_t ret;
> >> +
> >> +	if (!device_path) {
> >> +		con_out->output_string(con_out, L"<none>\r\n");
> >> +		return EFI_SUCCESS;
> >> +	}
> >> +
> >> +	string = dp2txt->convert_device_path_to_text(device_path, true,
> >false);
> >> +	if (!string) {
> >> +		con_out->output_string
> >> +			(con_out, L"Cannot convert device path to text\r\n");
> >> +		return EFI_OUT_OF_RESOURCES;
> >> +	}
> >> +	con_out->output_string(con_out, string);
> >> +	con_out->output_string(con_out, L"\r\n");
> >> +	ret = boottime->free_pool(string);
> >> +	if (ret != EFI_SUCCESS) {
> >> +		con_out->output_string(con_out, L"Cannot free pool memory\r\n");
> >> +		return ret;
> >> +	}
> >> +	return EFI_SUCCESS;
> >> +}
> >> +
> >> +/**
> >> + * efi_main() - entry point of the EFI application.
> >> + *
> >> + * @handle:	handle of the loaded image
> >> + * @systab:	system table
> >> + * @return:	status code
> >> + */
> >> +efi_status_t EFIAPI efi_main(efi_handle_t handle,
> >> +			     struct efi_system_table *systab)
> >> +{
> >> +	struct efi_loaded_image *loaded_image;
> >> +	struct efi_device_path_to_text_protocol *device_path_to_text;
> >> +	struct efi_device_path *device_path;
> >> +	efi_status_t ret;
> >> +
> >> +	systable = systab;
> >> +	boottime = systable->boottime;
> >> +	con_out = systable->con_out;
> >> +
> >> +	/* UEFI requires CR LF */
> >> +	con_out->output_string(con_out, L"Hello, world!\r\n");
> >> +
> >> +	print_uefi_revision();
> >> +	print_config_tables();
> >> +
> >> +	/* Get the loaded image protocol */
> >> +	ret = boottime->handle_protocol(handle, &loaded_image_guid,
> >> +					(void **)&loaded_image);
> >> +	if (ret != EFI_SUCCESS) {
> >> +		con_out->output_string
> >> +			(con_out, L"Cannot open loaded image protocol\r\n");
> >> +		goto out;
> >> +	}
> >> +	print_load_options(loaded_image);
> >> +
> >> +	/* Get the device path to text protocol */
> >> +	ret = boottime->locate_protocol(&device_path_to_text_protocol_guid,
> >> +					NULL, (void **)&device_path_to_text);
> >> +	if (ret != EFI_SUCCESS) {
> >> +		con_out->output_string
> >> +			(con_out, L"Cannot open device path to text protocol\r\n");
> >> +		goto out;
> >> +	}
> >> +	if (!loaded_image->device_handle) {
> >> +		con_out->output_string
> >> +			(con_out, L"Missing device handle\r\n");
> >> +		goto out;
> >> +	}
> >> +	ret = boottime->handle_protocol(loaded_image->device_handle,
> >> +					&device_path_guid,
> >> +					(void **)&device_path);
> >> +	if (ret != EFI_SUCCESS) {
> >> +		con_out->output_string
> >> +			(con_out, L"Missing devide path for device handle\r\n");
> >> +		goto out;
> >> +	}
> >> +	con_out->output_string(con_out, L"Boot device: ");
> >> +	ret = print_device_path(device_path, device_path_to_text);
> >> +	if (ret != EFI_SUCCESS)
> >> +		goto out;
> >> +	con_out->output_string(con_out, L"File path: ");
> >> +	ret = print_device_path(loaded_image->file_path,
> >device_path_to_text);
> >> +	if (ret != EFI_SUCCESS)
> >> +		goto out;
> >> 
> >>  out:
> >>  	boottime->exit(handle, ret, 0, NULL);
> >> --
> >> 2.29.2
> >> 
> 

[PATCH 1/3] efi_loader: print boot device and file path in helloworld

[Heinrich Schuchardt]

On 15.01.21 05:29, AKASHI Takahiro wrote:
> On Fri, Jan 15, 2021 at 04:12:18AM +0100, Heinrich Schuchardt wrote:
>> Am 15. Januar 2021 02:56:03 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>>> Heinrich,
>>>
>>> On Tue, Jan 12, 2021 at 08:58:40PM +0100, Heinrich Schuchardt wrote:
>>>> Let helloworld.efi print the device path of the boot device and the
>>> file
>>>> path as provided by the loaded image protocol.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>  lib/efi_loader/helloworld.c | 167
>>> +++++++++++++++++++++++++++++-------

<snip />

>>>
>>> If this kind of information is quite useful for users, why not add
>>> that (printing) feature as an option of bootefi (or efidebug)?
>>> I'm afraid that most users who are irritated as you said won't be able
>>> to imagine such information be printed by helloworld app.
>>>
>>
>> The file path is written in
>>
>> https://github.com/trini/u-boot/blob/master/cmd/bootefi.c#L471
>>
>> Device paths are not really user friendly.
>
> So why do you want to print such info at helloworld?
>
> I guess that, according to your cover letter, you have in your mind
> some cases where an user may get in trouble relating to the boot device.
> Right?
>
>> So I would not like to write it there.
>
> What I meant to suggest is to add an option, -v or -h, to bootefi,
> which prints verbose (and helpful) information for users to identify a cause.
> I can easily imagine users may blindly try to add -[v|h] when
> they see an error message even if they don't know there is such an option:)

To me helloworld.efi is a tool for a developer to see if an EFI binary
is correctly invoked.

The normal U-Boot code we want to keep as slim as possible.

According to the spec UEFI boots from the ESP and typically there is
only one. So printing the file path in cmd/bootefi should be enough.

Best regards

Heinrich

[PATCH 1/3] efi_loader: print boot device and file path in helloworld

[AKASHI Takahiro]

On Fri, Jan 15, 2021 at 01:02:51PM +0100, Heinrich Schuchardt wrote:
> On 15.01.21 05:29, AKASHI Takahiro wrote:
> > On Fri, Jan 15, 2021 at 04:12:18AM +0100, Heinrich Schuchardt wrote:
> >> Am 15. Januar 2021 02:56:03 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> >>> Heinrich,
> >>>
> >>> On Tue, Jan 12, 2021 at 08:58:40PM +0100, Heinrich Schuchardt wrote:
> >>>> Let helloworld.efi print the device path of the boot device and the
> >>> file
> >>>> path as provided by the loaded image protocol.
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>> ---
> >>>>  lib/efi_loader/helloworld.c | 167
> >>> +++++++++++++++++++++++++++++-------
> 
> <snip />
> 
> >>>
> >>> If this kind of information is quite useful for users, why not add
> >>> that (printing) feature as an option of bootefi (or efidebug)?
> >>> I'm afraid that most users who are irritated as you said won't be able
> >>> to imagine such information be printed by helloworld app.
> >>>
> >>
> >> The file path is written in
> >>
> >> https://github.com/trini/u-boot/blob/master/cmd/bootefi.c#L471
> >>
> >> Device paths are not really user friendly.
> >
> > So why do you want to print such info at helloworld?
> >
> > I guess that, according to your cover letter, you have in your mind
> > some cases where an user may get in trouble relating to the boot device.
> > Right?
> >
> >> So I would not like to write it there.
> >
> > What I meant to suggest is to add an option, -v or -h, to bootefi,
> > which prints verbose (and helpful) information for users to identify a cause.
> > I can easily imagine users may blindly try to add -[v|h] when
> > they see an error message even if they don't know there is such an option:)
> 
> To me helloworld.efi is a tool for a developer to see if an EFI binary
> is correctly invoked.

My point is that most users (developers?) don't intuitively imagine
such information will be printed with helloworld app.

> The normal U-Boot code we want to keep as slim as possible.

(I doubt this in terms of UEFI)

> According to the spec UEFI boots from the ESP and typically there is
> only one. So printing the file path in cmd/bootefi should be enough.

So again,
> > So why do you want to print such info at helloworld?

-Takahiro Akashi


> Best regards
> 
> Heinrich

[PATCH 2/3] efi_loader: carve out efi_check_pe()

[Heinrich Schuchardt]

Carve out a function to check that a buffer contains a PE-COFF image.

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

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0fc2255f3f..63459ea649 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -453,6 +453,8 @@ efi_status_t efi_set_watchdog(unsigned long timeout);

 /* Called from places to check whether a timer expired */
 void efi_timer_check(void);
+/* Check if a buffer contains a PE-COFF image */
+efi_status_t efi_check_pe(void *buffer, size_t size, void **nt_header);
 /* PE loader implementation */
 efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 			 void *efi, size_t efi_size,
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 94f76ef6b8..d4dd9e9433 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -675,6 +675,46 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 }
 #endif /* CONFIG_EFI_SECURE_BOOT */

+
+/**
+ * efi_check_pe() - check if a memory buffer contains a PE-COFF image
+ *
+ * @buffer:	buffer to check
+ * @size:	size of buffer
+ * @nt_header:	on return pointer to NT header of PE-COFF image
+ * Return:	EFI_SUCCESS if the buffer contains a PE-COFF image
+ */
+efi_status_t efi_check_pe(void *buffer, size_t size, void **nt_header)
+{
+	IMAGE_DOS_HEADER *dos = buffer;
+	IMAGE_NT_HEADERS32 *nt;
+
+	if (size < sizeof(*dos))
+		return EFI_INVALID_PARAMETER;
+
+	/* Check for DOS magix */
+	if (dos->e_magic != IMAGE_DOS_SIGNATURE)
+		return EFI_INVALID_PARAMETER;
+
+	/*
+	 * Check if the image section header fits into the file. Knowing that at
+	 * least one section header follows we only need to check for the length
+	 * of the 64bit header which is longer than the 32bit header.
+	 */
+	if (size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS32))
+		return EFI_INVALID_PARAMETER;
+	nt = (IMAGE_NT_HEADERS32 *)((u8 *)buffer + dos->e_lfanew);
+
+	/* Check for PE-COFF magic */
+	if (nt->Signature != IMAGE_NT_SIGNATURE)
+		return EFI_INVALID_PARAMETER;
+
+	if (nt_header)
+		*nt_header = nt;
+
+	return EFI_SUCCESS;
+}
+
 /**
  * efi_load_pe() - relocate EFI binary
  *
@@ -705,36 +745,10 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 	int supported = 0;
 	efi_status_t ret;

-	/* Sanity check for a file header */
-	if (efi_size < sizeof(*dos)) {
-		log_err("Truncated DOS Header\n");
-		ret = EFI_LOAD_ERROR;
-		goto err;
-	}
-
-	dos = efi;
-	if (dos->e_magic != IMAGE_DOS_SIGNATURE) {
-		log_err("Invalid DOS Signature\n");
-		ret = EFI_LOAD_ERROR;
-		goto err;
-	}
-
-	/*
-	 * Check if the image section header fits into the file. Knowing that at
-	 * least one section header follows we only need to check for the length
-	 * of the 64bit header which is longer than the 32bit header.
-	 */
-	if (efi_size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS64)) {
-		log_err("Invalid offset for Extended Header\n");
-		ret = EFI_LOAD_ERROR;
-		goto err;
-	}
-
-	nt = (void *) ((char *)efi + dos->e_lfanew);
-	if (nt->Signature != IMAGE_NT_SIGNATURE) {
-		log_err("Invalid NT Signature\n");
-		ret = EFI_LOAD_ERROR;
-		goto err;
+	ret = efi_check_pe(efi, efi_size, (void **)&nt);
+	if (ret != EFI_SUCCESS) {
+		log_err("Not a PE-COFF file\n");
+		return EFI_LOAD_ERROR;
 	}

 	for (i = 0; machines[i]; i++)
@@ -746,8 +760,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 	if (!supported) {
 		log_err("Machine type 0x%04x is not supported\n",
 			nt->FileHeader.Machine);
-		ret = EFI_LOAD_ERROR;
-		goto err;
+		return EFI_LOAD_ERROR;
 	}

 	num_sections = nt->FileHeader.NumberOfSections;
@@ -757,8 +770,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 	if (efi_size < ((void *)sections + sizeof(sections[0]) * num_sections
 			- efi)) {
 		log_err("Invalid number of sections: %d\n", num_sections);
-		ret = EFI_LOAD_ERROR;
-		goto err;
+		return EFI_LOAD_ERROR;
 	}

 	/* Authenticate an image */
--
2.29.2

[PATCH 2/3] efi_loader: carve out efi_check_pe()

[Simon Glass]

On Tue, 12 Jan 2021 at 12:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Carve out a function to check that a buffer contains a PE-COFF image.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/efi_loader.h              |  2 +
>  lib/efi_loader/efi_image_loader.c | 80 ++++++++++++++++++-------------
>  2 files changed, 48 insertions(+), 34 deletions(-)
>

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

[PATCH 3/3] efi_loader: setting boot device

[Heinrich Schuchardt]

Up to now the bootefi command used the last file loaded to determine the
boot partition. This has led to errors when the fdt had been loaded from
another partition after the EFI binary.

Before setting the boot device from a loaded file check if it is a PE-COFF
image or a FIT image.

For a PE-COFF image remember address and size, boot device and path.

For a FIT image remember boot device and path.

If the PE-COFF image is overwritten by loading another file, forget it.

Do not allow to start an image via bootefi which is not the last loaded
PE-COFF image.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/bootefi.c        | 136 ++++++++++++++++++++++++++-----------------
 doc/uefi/uefi.rst    |  11 ++--
 fs/fs.c              |   3 +-
 include/efi_loader.h |   6 +-
 net/tftp.c           |   9 ++-
 5 files changed, 98 insertions(+), 67 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index c82a5bacf6..bd0d12ea9b 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -29,6 +29,78 @@ DECLARE_GLOBAL_DATA_PTR;

 static struct efi_device_path *bootefi_image_path;
 static struct efi_device_path *bootefi_device_path;
+static void *image_addr;
+static size_t image_size;
+
+/**
+ * efi_clear_bootdev() - clear boot device
+ */
+static void efi_clear_bootdev(void)
+{
+	efi_free_pool(bootefi_device_path);
+	efi_free_pool(bootefi_image_path);
+	bootefi_device_path = NULL;
+	bootefi_image_path = NULL;
+	image_addr = NULL;
+	image_size = 0;
+}
+
+/**
+ * efi_set_bootdev() - set boot device
+ *
+ * This function is called when a file is loaded, e.g. via the 'load' command.
+ * We use the path to this file to inform the UEFI binary about the boot device.
+ *
+ * @dev:		device, e.g. "MMC"
+ * @devnr:		number of the device, e.g. "1:2"
+ * @path:		path to file loaded
+ * @buffer:		buffer with file loaded
+ * @buffer_size:	size of file loaded
+ */
+void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
+		     void *buffer, size_t buffer_size)
+{
+	struct efi_device_path *device, *image;
+	efi_status_t ret;
+
+	/* Forget overwritten image */
+	if (buffer + buffer_size >= image_addr &&
+	    image_addr + image_size >= buffer)
+		efi_clear_bootdev();
+
+	/* Remember only PE-COFF and FIT images */
+	if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) {
+		if (!fit_check_format(buffer))
+			return;
+		/*
+		 * FIT images of type EFI_OS are started via command bootm.
+		 * We should not use their boot device with the bootefi command.
+		 */
+		buffer = 0;
+		buffer_size = 0;
+	}
+
+	/* efi_set_bootdev() is typically called repeatedly, recover memory */
+	efi_clear_bootdev();
+
+	image_addr = buffer;
+	image_size = buffer_size;
+
+	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
+	if (ret == EFI_SUCCESS) {
+		bootefi_device_path = device;
+		if (image) {
+			/* FIXME: image should not contain device */
+			struct efi_device_path *image_tmp = image;
+
+			efi_dp_split_file_path(image, &device, &image);
+			efi_free_pool(image_tmp);
+		}
+		bootefi_image_path = image;
+	} else {
+		efi_clear_bootdev();
+	}
+}

 /**
  * efi_env_set_load_options() - set load options from environment variable
@@ -398,33 +470,28 @@ static int do_bootefi_image(const char *image_opt)
 {
 	void *image_buf;
 	unsigned long addr, size;
-	const char *size_str;
 	efi_status_t ret;

 #ifdef CONFIG_CMD_BOOTEFI_HELLO
 	if (!strcmp(image_opt, "hello")) {
 		image_buf = __efi_helloworld_begin;
 		size = __efi_helloworld_end - __efi_helloworld_begin;
-
-		efi_free_pool(bootefi_device_path);
-		efi_free_pool(bootefi_image_path);
-		bootefi_device_path = NULL;
-		bootefi_image_path = NULL;
+		efi_clear_bootdev();
 	} else
 #endif
 	{
-		size_str = env_get("filesize");
-		if (size_str)
-			size = simple_strtoul(size_str, NULL, 16);
-		else
-			size = 0;
-
-		addr = simple_strtoul(image_opt, NULL, 16);
+		addr = strtoul(image_opt, NULL, 16);
 		/* Check that a numeric value was passed */
-		if (!addr && *image_opt != '0')
+		if (!addr)
 			return CMD_RET_USAGE;

 		image_buf = map_sysmem(addr, size);
+
+		if (image_buf != image_addr) {
+			log_err("No UEFI binary known at %s\n", image_opt);
+			return CMD_RET_FAILURE;
+		}
+		size = image_size;
 	}
 	ret = efi_run_image(image_buf, size);

@@ -557,11 +624,8 @@ static efi_status_t bootefi_test_prepare
 	if (ret == EFI_SUCCESS)
 		return ret;

-	efi_free_pool(bootefi_image_path);
-	bootefi_image_path = NULL;
 failure:
-	efi_free_pool(bootefi_device_path);
-	bootefi_device_path = NULL;
+	efi_clear_bootdev();
 	return ret;
 }

@@ -681,39 +745,3 @@ U_BOOT_CMD(
 	"Boots an EFI payload from memory",
 	bootefi_help_text
 );
-
-/**
- * efi_set_bootdev() - set boot device
- *
- * This function is called when a file is loaded, e.g. via the 'load' command.
- * We use the path to this file to inform the UEFI binary about the boot device.
- *
- * @dev:	device, e.g. "MMC"
- * @devnr:	number of the device, e.g. "1:2"
- * @path:	path to file loaded
- */
-void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
-{
-	struct efi_device_path *device, *image;
-	efi_status_t ret;
-
-	/* efi_set_bootdev is typically called repeatedly, recover memory */
-	efi_free_pool(bootefi_device_path);
-	efi_free_pool(bootefi_image_path);
-
-	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
-	if (ret == EFI_SUCCESS) {
-		bootefi_device_path = device;
-		if (image) {
-			/* FIXME: image should not contain device */
-			struct efi_device_path *image_tmp = image;
-
-			efi_dp_split_file_path(image, &device, &image);
-			efi_free_pool(image_tmp);
-		}
-		bootefi_image_path = image;
-	} else {
-		bootefi_device_path = NULL;
-		bootefi_image_path = NULL;
-	}
-}
diff --git a/doc/uefi/uefi.rst b/doc/uefi/uefi.rst
index dc930d9240..5a67737c15 100644
--- a/doc/uefi/uefi.rst
+++ b/doc/uefi/uefi.rst
@@ -59,13 +59,10 @@ Below you find the output of an example session starting GRUB::
     120832 bytes read in 7 ms (16.5 MiB/s)
     => bootefi ${kernel_addr_r} ${fdt_addr_r}

-The bootefi command uses the device, the file name, and the file size
-(environment variable 'filesize') of the most recently loaded file when setting
-up the binary for execution. So the UEFI binary should be loaded last.
-
-The environment variable 'bootargs' is passed as load options in the UEFI system
-table. The Linux kernel EFI stub uses the load options as command line
-arguments.
+When booting from a memory location it is unknown from which file it was loaded.
+Therefore the bootefi command uses the device path of the block device partition
+or the network adapter and the file name of the most recently loaded PE-COFF
+file when setting up the loaded image protocol.

 Launching a UEFI binary from a FIT image
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/fs/fs.c b/fs/fs.c
index 7a4020607a..a3989f9bdb 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -752,7 +752,8 @@ int do_load(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[],

 	if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
 		efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
-				(argc > 4) ? argv[4] : "");
+				(argc > 4) ? argv[4] : "", map_sysmem(addr, 0),
+				len_read);

 	printf("%llu bytes read in %lu ms", len_read, time);
 	if (time > 0) {
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 63459ea649..2465a47a91 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -467,7 +467,8 @@ void efi_restore_gd(void);
 /* Call this to relocate the runtime section to an address space */
 void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
 /* Call this to set the current device name */
-void efi_set_bootdev(const char *dev, const char *devnr, const char *path);
+void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
+		     void *buffer, size_t buffer_size);
 /* Add a new object to the object list. */
 void efi_add_handle(efi_handle_t obj);
 /* Create handle */
@@ -829,7 +830,8 @@ static inline efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len)
 /* No loader configured, stub out EFI_ENTRY */
 static inline void efi_restore_gd(void) { }
 static inline void efi_set_bootdev(const char *dev, const char *devnr,
-				   const char *path) { }
+				   const char *path, void *buffer,
+				   size_t buffer_size) { }
 static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
 static inline void efi_print_image_infos(void *pc) { }

diff --git a/net/tftp.c b/net/tftp.c
index 6fdb1a821a..2cfa0b1486 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -329,6 +329,12 @@ static void tftp_complete(void)
 			time_start * 1000, "/s");
 	}
 	puts("\ndone\n");
+	if (IS_ENABLED(CONFIG_CMD_BOOTEFI)) {
+		if (!tftp_put_active)
+			efi_set_bootdev("Net", "", tftp_filename,
+					map_sysmem(tftp_load_addr, 0),
+					net_boot_file_size);
+	}
 	net_set_state(NETLOOP_SUCCESS);
 }

@@ -841,9 +847,6 @@ void tftp_start(enum proto_t protocol)
 		printf("Load address: 0x%lx\n", tftp_load_addr);
 		puts("Loading: *\b");
 		tftp_state = STATE_SEND_RRQ;
-#ifdef CONFIG_CMD_BOOTEFI
-		efi_set_bootdev("Net", "", tftp_filename);
-#endif
 	}

 	time_start = get_timer(0);
--
2.29.2

[PATCH 3/3] efi_loader: setting boot device

[Simon Glass]

Hi Heinrich,

On Tue, 12 Jan 2021 at 12:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Up to now the bootefi command used the last file loaded to determine the
> boot partition. This has led to errors when the fdt had been loaded from
> another partition after the EFI binary.
>
> Before setting the boot device from a loaded file check if it is a PE-COFF
> image or a FIT image.
>
> For a PE-COFF image remember address and size, boot device and path.
>
> For a FIT image remember boot device and path.
>
> If the PE-COFF image is overwritten by loading another file, forget it.
>
> Do not allow to start an image via bootefi which is not the last loaded
> PE-COFF image.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  cmd/bootefi.c        | 136 ++++++++++++++++++++++++++-----------------
>  doc/uefi/uefi.rst    |  11 ++--
>  fs/fs.c              |   3 +-
>  include/efi_loader.h |   6 +-
>  net/tftp.c           |   9 ++-
>  5 files changed, 98 insertions(+), 67 deletions(-)
>

Does this need an update to the tests?

Regards,
Simon

[PATCH 3/3] efi_loader: setting boot device

[Heinrich Schuchardt]

On 1/19/21 7:06 PM, Simon Glass wrote:
> Hi Heinrich,
>
> On Tue, 12 Jan 2021 at 12:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Up to now the bootefi command used the last file loaded to determine the
>> boot partition. This has led to errors when the fdt had been loaded from
>> another partition after the EFI binary.
>>
>> Before setting the boot device from a loaded file check if it is a PE-COFF
>> image or a FIT image.
>>
>> For a PE-COFF image remember address and size, boot device and path.
>>
>> For a FIT image remember boot device and path.
>>
>> If the PE-COFF image is overwritten by loading another file, forget it.
>>
>> Do not allow to start an image via bootefi which is not the last loaded
>> PE-COFF image.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   cmd/bootefi.c        | 136 ++++++++++++++++++++++++++-----------------
>>   doc/uefi/uefi.rst    |  11 ++--
>>   fs/fs.c              |   3 +-
>>   include/efi_loader.h |   6 +-
>>   net/tftp.c           |   9 ++-
>>   5 files changed, 98 insertions(+), 67 deletions(-)
>>
>
> Does this need an update to the tests?

Our current tests still run successfully.

The observed problematic behavior is not covered by our tests. To run
GRUB or helloworld.efi we just load one file and execute it.

So it would make sense to extend the coverage of our tests with a future
patch.

Best regards

Heinrich

>
> Regards,
> Simon
>

[PATCH 3/3] efi_loader: setting boot device

[Simon Glass]

Hi Heinrich,

On Tue, 19 Jan 2021 at 11:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 1/19/21 7:06 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Tue, 12 Jan 2021 at 12:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Up to now the bootefi command used the last file loaded to determine the
> >> boot partition. This has led to errors when the fdt had been loaded from
> >> another partition after the EFI binary.
> >>
> >> Before setting the boot device from a loaded file check if it is a PE-COFF
> >> image or a FIT image.
> >>
> >> For a PE-COFF image remember address and size, boot device and path.
> >>
> >> For a FIT image remember boot device and path.
> >>
> >> If the PE-COFF image is overwritten by loading another file, forget it.
> >>
> >> Do not allow to start an image via bootefi which is not the last loaded
> >> PE-COFF image.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>   cmd/bootefi.c        | 136 ++++++++++++++++++++++++++-----------------
> >>   doc/uefi/uefi.rst    |  11 ++--
> >>   fs/fs.c              |   3 +-
> >>   include/efi_loader.h |   6 +-
> >>   net/tftp.c           |   9 ++-
> >>   5 files changed, 98 insertions(+), 67 deletions(-)
> >>
> >
> > Does this need an update to the tests?
>
> Our current tests still run successfully.
>
> The observed problematic behavior is not covered by our tests. To run
> GRUB or helloworld.efi we just load one file and execute it.
>
> So it would make sense to extend the coverage of our tests with a future
> patch.

Yes indeed.

Regards,
Simon

Re: [PATCH 3/3] efi_loader: setting boot device

[Kyle Evans]

 On Tue, Jan 12, 2021 at 1:59 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Up to now the bootefi command used the last file loaded to determine the
> boot partition. This has led to errors when the fdt had been loaded from
> another partition after the EFI binary.
>
> Before setting the boot device from a loaded file check if it is a PE-COFF
> image or a FIT image.
>
> For a PE-COFF image remember address and size, boot device and path.
>
> For a FIT image remember boot device and path.
>
> If the PE-COFF image is overwritten by loading another file, forget it.
>
> Do not allow to start an image via bootefi which is not the last loaded
> PE-COFF image.
>

Hi,

I'm only a little late on this, but may I ask what the rationale of
this last part is? I'm afraid there are some real-world use cases
where a compromise would be great, allowing bootefi to accept a random
region of memory to boot -- in my case, I have the payload (FreeBSD's
loader.efi) already in memory when U-Boot starts and it's unclear that
I can come up with some other way to boot it that doesn't involve a
lot of backflips.

My specific suggestion, which I can formally post to the list if you
don't immediately object, is this:
https://people.freebsd.org/~kevans/uboot.patch

It basically adds another form:

`bootefi addr [fdt [size]]`

If $addr isn't the last loaded PE-COFF, size must be provided. fdt can
be `-` to signal EFI_FDT_USE_INTERNAL. If we provide an address that
isn't the last loaded PE-COFF, it wipes out the device path and lets
efi_run_image() synthesize it. Providing a size with the address of
the last loaded PE-COFF is an error, but that's a bit arbitrary.

Obviously our loader doesn't get good source disk information this way
and I have to drive it manually, but that's still orders of magnitude
better than having to locally shuffle new loader.efis onto a flash
drive and keep moving the flash drive back and forth to iterate on it.

Thans,

Kyle Evans

Re: [PATCH 3/3] efi_loader: setting boot device

[Heinrich Schuchardt]

Am 3. April 2022 23:08:33 MESZ schrieb Kyle Evans <kevans@freebsd.org>:
> On Tue, Jan 12, 2021 at 1:59 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Up to now the bootefi command used the last file loaded to determine the
>> boot partition. This has led to errors when the fdt had been loaded from
>> another partition after the EFI binary.
>>
>> Before setting the boot device from a loaded file check if it is a PE-COFF
>> image or a FIT image.
>>
>> For a PE-COFF image remember address and size, boot device and path.
>>
>> For a FIT image remember boot device and path.
>>
>> If the PE-COFF image is overwritten by loading another file, forget it.
>>
>> Do not allow to start an image via bootefi which is not the last loaded
>> PE-COFF image.
>>
>
>Hi,
>
>I'm only a little late on this, but may I ask what the rationale of
>this last part is? I'm afraid there are some real-world use cases
>where a compromise would be great, allowing bootefi to accept a random
>region of memory to boot -- in my case, I have the payload (FreeBSD's
>loader.efi) already in memory when U-Boot starts and it's unclear that

Could you, please, describe your use case in some more detail.

Why can't you load loader.efi from the ESP?

>I can come up with some other way to boot it that doesn't involve a
>lot of backflips.
>My specific suggestion, which I can formally post to the list if you
>don't immediately object, is this:
>https://people.freebsd.org/~kevans/uboot.patch
>
>It basically adds another form:
>
>`bootefi addr [fdt [size]]`

What should size be used for?
Both EFI binaries and device-trees provide their size in a header field.

Best regards

Heinrich

>
>If $addr isn't the last loaded PE-COFF, size must be provided. fdt can
>be `-` to signal EFI_FDT_USE_INTERNAL. If we provide an address that
>isn't the last loaded PE-COFF, it wipes out the device path and lets
>efi_run_image() synthesize it. Providing a size with the address of
>the last loaded PE-COFF is an error, but that's a bit arbitrary.
>
>Obviously our loader doesn't get good source disk information this way
>and I have to drive it manually, but that's still orders of magnitude
>better than having to locally shuffle new loader.efis onto a flash
>drive and keep moving the flash drive back and forth to iterate on it.
>
>Thans,
>
>Kyle Evans

Re: [PATCH 3/3] efi_loader: setting boot device

[Kyle Evans]

On Mon, Apr 4, 2022 at 12:09 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 3. April 2022 23:08:33 MESZ schrieb Kyle Evans <kevans@freebsd.org>:
> > On Tue, Jan 12, 2021 at 1:59 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Up to now the bootefi command used the last file loaded to determine the
> >> boot partition. This has led to errors when the fdt had been loaded from
> >> another partition after the EFI binary.
> >>
> >> Before setting the boot device from a loaded file check if it is a PE-COFF
> >> image or a FIT image.
> >>
> >> For a PE-COFF image remember address and size, boot device and path.
> >>
> >> For a FIT image remember boot device and path.
> >>
> >> If the PE-COFF image is overwritten by loading another file, forget it.
> >>
> >> Do not allow to start an image via bootefi which is not the last loaded
> >> PE-COFF image.
> >>
> >
> >Hi,
> >
> >I'm only a little late on this, but may I ask what the rationale of
> >this last part is? I'm afraid there are some real-world use cases
> >where a compromise would be great, allowing bootefi to accept a random
> >region of memory to boot -- in my case, I have the payload (FreeBSD's
> >loader.efi) already in memory when U-Boot starts and it's unclear that
>
> Could you, please, describe your use case in some more detail.
>
> Why can't you load loader.efi from the ESP?
>

I'm explicitly trying to override the loader.efi from ESP, because
it's broken and I can't (easily) keep switching it out. This is on
Apple's M1, so I can happily inject the new loader.efi from m1n1 (much
like the JTAG use-case mentioned in this file) for testing new
iterations.

> >I can come up with some other way to boot it that doesn't involve a
> >lot of backflips.
> >My specific suggestion, which I can formally post to the list if you
> >don't immediately object, is this:
> >https://people.freebsd.org/~kevans/uboot.patch
> >
> >It basically adds another form:
> >
> >`bootefi addr [fdt [size]]`
>
> What should size be used for?
> Both EFI binaries and device-trees provide their size in a header field.
>

Right now it's used because efi_run_image() wants the buffer size to
construct the memory-based device path. I'm not familiar enough with
U-Boot to know if there's a sensible API for grabbing the size from
this PE image in memory.

Re: [PATCH 3/3] efi_loader: setting boot device

[Heinrich Schuchardt]

Am 4. April 2022 07:40:16 MESZ schrieb Kyle Evans <kevans@freebsd.org>:
>On Mon, Apr 4, 2022 at 12:09 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Am 3. April 2022 23:08:33 MESZ schrieb Kyle Evans <kevans@freebsd.org>:
>> > On Tue, Jan 12, 2021 at 1:59 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> >>
>> >> Up to now the bootefi command used the last file loaded to determine the
>> >> boot partition. This has led to errors when the fdt had been loaded from
>> >> another partition after the EFI binary.
>> >>
>> >> Before setting the boot device from a loaded file check if it is a PE-COFF
>> >> image or a FIT image.
>> >>
>> >> For a PE-COFF image remember address and size, boot device and path.
>> >>
>> >> For a FIT image remember boot device and path.
>> >>
>> >> If the PE-COFF image is overwritten by loading another file, forget it.
>> >>
>> >> Do not allow to start an image via bootefi which is not the last loaded
>> >> PE-COFF image.
>> >>
>> >
>> >Hi,
>> >
>> >I'm only a little late on this, but may I ask what the rationale of
>> >this last part is? I'm afraid there are some real-world use cases
>> >where a compromise would be great, allowing bootefi to accept a random
>> >region of memory to boot -- in my case, I have the payload (FreeBSD's
>> >loader.efi) already in memory when U-Boot starts and it's unclear that
>>
>> Could you, please, describe your use case in some more detail.
>>
>> Why can't you load loader.efi from the ESP?
>>
>
>I'm explicitly trying to override the loader.efi from ESP, because
>it's broken and I can't (easily) keep switching it out. This is on
>Apple's M1, so I can happily inject the new loader.efi from m1n1 (much
>like the JTAG use-case mentioned in this file) for testing new
>iterations.

You could use the loady command to load the EFI binary via the UART. This should work with an unpatched U-Boot v2022.04-rc5.

>
>> >I can come up with some other way to boot it that doesn't involve a
>> >lot of backflips.
>> >My specific suggestion, which I can formally post to the list if you
>> >don't immediately object, is this:
>> >https://people.freebsd.org/~kevans/uboot.patch
>> >
>> >It basically adds another form:
>> >
>> >`bootefi addr [fdt [size]]`
>>
>> What should size be used for?
>> Both EFI binaries and device-trees provide their size in a header field.
>>

Please, send your patch for review to the mailing list and me.

Best regards

Heinrich

>
>Right now it's used because efi_run_image() wants the buffer size to
>construct the memory-based device path. I'm not familiar enough with
>U-Boot to know if there's a sensible API for grabbing the size from
>this PE image in memory.

Re: [PATCH 3/3] efi_loader: setting boot device

[Kyle Evans]

On Mon, Apr 4, 2022 at 12:59 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 4. April 2022 07:40:16 MESZ schrieb Kyle Evans <kevans@freebsd.org>:
> >On Mon, Apr 4, 2022 at 12:09 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Am 3. April 2022 23:08:33 MESZ schrieb Kyle Evans <kevans@freebsd.org>:
> >> > On Tue, Jan 12, 2021 at 1:59 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >> >>
> >> >> Up to now the bootefi command used the last file loaded to determine the
> >> >> boot partition. This has led to errors when the fdt had been loaded from
> >> >> another partition after the EFI binary.
> >> >>
> >> >> Before setting the boot device from a loaded file check if it is a PE-COFF
> >> >> image or a FIT image.
> >> >>
> >> >> For a PE-COFF image remember address and size, boot device and path.
> >> >>
> >> >> For a FIT image remember boot device and path.
> >> >>
> >> >> If the PE-COFF image is overwritten by loading another file, forget it.
> >> >>
> >> >> Do not allow to start an image via bootefi which is not the last loaded
> >> >> PE-COFF image.
> >> >>
> >> >
> >> >Hi,
> >> >
> >> >I'm only a little late on this, but may I ask what the rationale of
> >> >this last part is? I'm afraid there are some real-world use cases
> >> >where a compromise would be great, allowing bootefi to accept a random
> >> >region of memory to boot -- in my case, I have the payload (FreeBSD's
> >> >loader.efi) already in memory when U-Boot starts and it's unclear that
> >>
> >> Could you, please, describe your use case in some more detail.
> >>
> >> Why can't you load loader.efi from the ESP?
> >>
> >
> >I'm explicitly trying to override the loader.efi from ESP, because
> >it's broken and I can't (easily) keep switching it out. This is on
> >Apple's M1, so I can happily inject the new loader.efi from m1n1 (much
> >like the JTAG use-case mentioned in this file) for testing new
> >iterations.
>
> You could use the loady command to load the EFI binary via the UART. This should work with an unpatched U-Boot v2022.04-rc5.
>

Indeed, thanks! It's a bit less convenient than being able to script
this from the beginning, but it's a lot better than having to
distribute a patched u-boot to folks working on this with me.

> >
> >> >I can come up with some other way to boot it that doesn't involve a
> >> >lot of backflips.
> >> >My specific suggestion, which I can formally post to the list if you
> >> >don't immediately object, is this:
> >> >https://people.freebsd.org/~kevans/uboot.patch
> >> >
> >> >It basically adds another form:
> >> >
> >> >`bootefi addr [fdt [size]]`
> >>
> >> What should size be used for?
> >> Both EFI binaries and device-trees provide their size in a header field.
> >>
>
> Please, send your patch for review to the mailing list and me.
>

Will do- I considered dropping the size or adding a subcommand that
does this instead and reading the size from the header, but I think
it's grown on me as a cheap/easy way to extend the base bootefi
without adding the footgun of a previously invalid command suddenly
doing the wrong thing. If you just plopped it into memory, knowing the
size should be an easy task